Re: [PATCH v2] sunrpc: simplify dprintk() macros and cleanup redundant debug guards

[Sean Chang]

On Tue, Mar 17, 2026 at 10:20 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
> > > > > Does this patch fixes also 202603110038.P6d14oxa-lkp@xxxxxxxxx?
> > > >
> > > > Regarding the LKP report:
> > > > I have reproduced the Sparse warning (void vs int mismatch) locally.
> > > > To resolve this, I'll use the do-while(0) form in v3 to ensure the macro
> > > > always evaluates to void:
> > > > -# define dfprintk(fac, ...)            no_printk(__VA_ARGS__)
> > > > -# define dfprintk_rcu(fac, ...)        no_printk(__VA_ARGS__)
> > > > +# define dfprintk(fac, ...)            do { no_printk(__VA_ARGS__); } while (0)
> > > > +# define dfprintk_rcu(fac, ...)        do { no_printk(__VA_ARGS__); } while (0)
> > >
> > > Wouldn't be better to drop ({}) in nfs_error*() macros?
> >
> > I understand that the ({}) wrapper might look redundant at first
> > glance. However,
> > even if I remove it, the return type remains an issue because no_printk() (which
> > dprintk() expands to) already contains its own ({}) statement expression.
> >
> > To resolve this without refactoring errorf(), which hasn't been
> > touched in years,
> > I believe modifying dfprintk() is the better path.
> >
> > One alternative is to explicitly force the return type to void like this:
> > ({ dprintk(fmt "\n", ##__VA_ARGS__); (void)0; })
> >
> > While this ensures the return type remains void (consistent with the
> > behavior before
> > dprintk() was redefined), it is admittedly clunky. We could wrap
> > (void)0 in a macro like
> > #define nothing_to_do ((void)0), but that adds unnecessary complexity.
> >
> > Therefore, I still prefer Option 1, as it restores the original
> > behavior from before the
> > recent commits and maintains the cleanest code structure for this subsystem.
> > What do you think?
>
> Personally I found the ({}) in nfs_error*() redundant and the point of those
> functions not being touched in years doesn't work for internal APIs. Do you
> know the reason why it wasn't touched? Perhaps there was nothing to do simply
> with that until dprintk() issue reveals some (legacy?) stuff to improve.
>
> I would not go with (void)0 approach, but I also think that dropping unneeded
> (if confirmed) expression wrapping is a good thing to do.
>

Hi Andy,
Thanks for your comment, how about removing the ({}) and ternary operator to
prevent sparse throw incompatible types. And use the do{} while(0) to prevent
dangling-else problem.

#define nfs_errorf(fc, fmt, ...) do { \
    if ((fc)->log.log) \
        errorf(fc, fmt, ## __VA_ARGS__); \
    else \
        dprintk(fmt "\n", ## __VA_ARGS__); \
} while (0)