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

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 12:21:41AM +0800, Sean Chang wrote:
> 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.

> 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.

LGTM.

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

-- 
With Best Regards,
Andy Shevchenko