Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards

[Fabio M. De Francesco]

On Tuesday, 30 January 2024 18:58:25 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:
> > On Tuesday, 30 January 2024 18:02:09 CET Dan Williams wrote:
> > > Fabio M. De Francesco wrote:
> > > > Add cond_guard() to conditional guards.
> > > > 
> > > > cond_guard() is used for the _interruptible(), _killable(), and _try
> > > > versions of locks. It expects a block where the failure can be handled
> > > > (e.g., calling printk() and returning -EINTR in case of failure).
> > > > 
> > > > As the other guards, it avoids to open code the release of the lock
> > > > after a goto to an 'out' label.
> > > > 
> > > > This remains an RFC because Dan suggested a slightly different syntax:
> > > > 	if (cond_guard(...))
> > > > 	
> > > > 		return -EINTR;
> > > > 
> > > > But the scoped_cond_guard() macro omits the if statement:
> > > >     	scoped_cond_guard (...) {
> > > >     	}
> > > > 
> > > > Thus define cond_guard() similarly to scoped_cond_guard() but with a
> > > > block
> > > > 
> > > > to handle the failure case:
> > > > 	cond_guard(...)
> > > > 	
> > > > 		return -EINTR;
> > > 
> > > That's too subtle for me, because of the mistakes that can be made with
> > > 
> > > brackets how about a syntax like:
> > >  	cond_guard(..., return -EINTR, ...)
> > > 
> > > ...to make it clear what happens if the lock acquisition fails without
> > > having to remember there is a hidden incomplete "if ()" statement in
> > > that macro? More below...
> > 
> > As you propose I can't see how to handle multi-line error path like in:
> > 	cond_guard(...) {
> > 	
> > 		dev_dbg(...);
> > 		return -EINTR;
> > 	
> > 	}
> 
> The _fail argument is a statement, to make it a compound statement maybe
> just add braces, something like:
> 
>     cond_guard(..., { dev_dbg(...); return -EINTR; }, ...)
> 
> ...another possibility is something like
> 
>     int rc = 0;
> 
>     cond_guard(..., rc = -EINTR, ...)
>     if (rc) {
>         ...
>         return rc;
>     }

I had tried this before sending this patch. It looked the most obvious 
solution. But it fails my tests: it always return -EINTR, regardless of the 
successful down.

It looks like it was not expanded as I was expecting.

Or my tests are wrong, but I can't see any obvious mistake.

BTW, it's interesting to notice that the following instead works. I guess that 
it is due to the same fact that required me to pass a pointer to 'rc' in the 
first version of this patch to (mistakenly) store the boolean of whether the 
constructor succeeded or failed.

	int rc;
	int *rcp = &rc;

	cond_guard(..., *rcp = -EINTR, ...)
	if (rc) {
		dev_dbg(...);
		return rc;
	}

This works but I think nobody wants to see anything like this.

Fabio
 
> 
> ...so, I don't think we need separate macros for the multi-statement
> case.