Re: [PATCH 0/3] cleanup: add if_not_cond_guard macro

[Jonathan Cameron]

On Tue, 1 Oct 2024 19:13:01 -0700
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:

> David Lechner wrote:
> > So far, I have not found scoped_cond_guard() to be nice to work with.
> > We have been using it quite a bit in the IIO subsystem via the
> > iio_device_claim_direct_scoped() macro.
> > 
> > The main thing I don't like is that scoped_cond_guard() uses a for loop
> > internally. In the IIO subsystem, we usually try to return as early as
> > possible, so often we are returning from all paths from withing this
> > hidden for loop. However, since it is a for loop, the compiler thinks
> > that it possible to exit the for loop and so we end up having to use
> > unreachable() after the end of the scope to avoid a compiler warning.
> > This is illustrated in the ad7380 patch in this series and there are 36
> > more instance of unreachable() already introduced in the IIO subsystem
> > because of this.
> > 
> > Also, scoped_cond_guard() is they only macro for conditional guards in
> > cleanup.h currently. This means that so far, patches adopting this are
> > generally converting something that wasn't scoped to be scoped. This
> > results in changing the indentation of a lot of lines of code, which is
> > just noise in the patches.
> > 
> > To avoid these issues, the natural thing to do would be to have a
> > non-scoped version of the scoped_cond_guard() macro. There was was a
> > rejected attempt to do this in [1], where one of the complaints was:
> >   
> > > > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > > > -       if (rc)
> > > > -               return rc;
> > > > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);  
> > >
> > > Yeah, this is an example of how NOT to do things.
> > >
> > > If you can't make the syntax be something clean and sane like
> > >
> > >         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
> > >                 return -EINTR;
> > >
> > > then this code should simply not be converted to guards AT ALL.  
> > 
> > [1]: https://lore.kernel.org/all/170905252721.2268463.6714121678946763402.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx/
> > 
> > I couldn't find a way to make a cond_guard() macro that would work like
> > exactly as suggested (the problem is that you can't declare a variable
> > in the condition expression of an if statement in C). So I am proposing
> > a macro that reads basically the same as the above so it still reads
> > almost like normal C code even though it hides the if statement a bit.
> > 
> >         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
> >                 return -EINTR;
> > 
> > The "not" is baked into the macro because in most cases, failing to
> > obtain the lock is the abnormal condition and generally we want to have
> > the abnormal path be the indented one.  
> 
> I think you could also take the "cond" out of the name because that is
> implied by the fact it's an 'if'.
> 
> So, calling this "if_not_guard ()", for the series, you can add:
> 
> Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> 
> ...but it's ultimately up to Peter and Linus if they find this "if ()"
> rename acceptable. 

This is a nice improvement to my eyes anyway and I hope will be fine
with Linus and Peter.  Whilst I like the cond guard stuff for the
simplifications it has brought in the IIO code, it is clunky in
some cases as you've pointed out.

Thanks for driving this forwards.


> If it is I would suggest the style should be treat it
> as an "if ()" statement and add this to .clang-format:
> 
> diff --git a/.clang-format b/.clang-format
> index 252820d9c80a..ae3511a69896 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -63,6 +63,8 @@ DerivePointerAlignment: false
>  DisableFormat: false
>  ExperimentalAutoDetectBinPacking: false
>  FixNamespaceComments: false
> +IfMacros:
> +  - 'if_not_guard'
>  
>  # Taken from:
>  #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \
> 
> 
> Last note, while the iio conversion looks correct to me, I would feel
> more comfortable if there was a way to have the compiler catch that
> plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
> iio_device_claim_direct_mode() as __must_check achieves that effect?

That would be good to catch. We've not had many missuses but there
have been one or two that have shown up in review.

Jonathan