Messages in this thread | | | From | "Fabio M. De Francesco" <> | Subject | Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards | Date | Wed, 31 Jan 2024 14:11:09 +0100 |
| |
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.
| |