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

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

[Fabio M. De Francesco]

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;

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
---
 drivers/cxl/core/region.c | 17 +++++------------
 include/linux/cleanup.h   | 13 +++++++++++++
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0f05692bfec3..20d405f01df5 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -666,28 +666,21 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled;
-	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	cond_guard(rwsem_read_intr, &cxl_region_rwsem)
+		return -EINTR;
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
+		return sysfs_emit(buf, "\n");
 	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
-
-	return rc;
+		return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int match_free_decoder(struct device *dev, void *data)
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..fc850e61a47e 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,15 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
+ * cond_guard(_name, args...):
+ * 	for conditional locks like mutex_trylock() or down_read_interruptible().
+ * 	It expects a block for handling errors like in the following example:
+ *
+ * 	cond_guard(rwsem_read_intr, &my_sem) {
+ * 		printk(KERN_NOTICE "Try failure in work0()\n");
+ * 		return -EINTR;
+ * 	}
+ *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -165,6 +174,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
+#define cond_guard(_name, args...) \
+	CLASS(_name, scope)(args); \
+	if (!__guard_ptr(_name)(&scope))
+
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
-- 
2.43.0

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

[Dan Williams]

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

> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
> ---
>  drivers/cxl/core/region.c | 17 +++++------------
>  include/linux/cleanup.h   | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0f05692bfec3..20d405f01df5 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -666,28 +666,21 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_endpoint_decoder *cxled;
> -	int rc;
>  
> -	rc = down_read_interruptible(&cxl_region_rwsem);
> -	if (rc)
> -		return rc;
> +	cond_guard(rwsem_read_intr, &cxl_region_rwsem)
> +		return -EINTR;
>  
>  	if (pos >= p->interleave_ways) {
>  		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
>  			p->interleave_ways);
> -		rc = -ENXIO;
> -		goto out;
> +		return -ENXIO;
>  	}
>  
>  	cxled = p->targets[pos];
>  	if (!cxled)
> -		rc = sysfs_emit(buf, "\n");
> +		return sysfs_emit(buf, "\n");
>  	else
> -		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
> -out:
> -	up_read(&cxl_region_rwsem);
> -
> -	return rc;
> +		return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
>  }
>  
>  static int match_free_decoder(struct device *dev, void *data)
> diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> index c2d09bc4f976..fc850e61a47e 100644
> --- a/include/linux/cleanup.h
> +++ b/include/linux/cleanup.h
> @@ -134,6 +134,15 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>   *	an anonymous instance of the (guard) class, not recommended for
>   *	conditional locks.
>   *
> + * cond_guard(_name, args...):
> + * 	for conditional locks like mutex_trylock() or down_read_interruptible().
> + * 	It expects a block for handling errors like in the following example:
> + *
> + * 	cond_guard(rwsem_read_intr, &my_sem) {
> + * 		printk(KERN_NOTICE "Try failure in work0()\n");
> + * 		return -EINTR;
> + * 	}
> + *
>   * scoped_guard (name, args...) { }:
>   *	similar to CLASS(name, scope)(args), except the variable (with the
>   *	explicit name 'scope') is declard in a for-loop such that its scope is
> @@ -165,6 +174,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
>  
>  #define __guard_ptr(_name) class_##_name##_lock_ptr
>  
> +#define cond_guard(_name, args...) \
> +	CLASS(_name, scope)(args); \
> +	if (!__guard_ptr(_name)(&scope))

This needs to protect against being used within another if () block.
Imagine a case of:

    if (...) {
    	cond_guard(...);
	    <statement>
    } else if (...)

...does that "else if" belong to the first "if ()" or the hidden one
inside the macro?

You can steal the embedded "if ()" trick from scoped_cond_guard() and do
something like (untested):

#define cond_guard(_name, _fail, args...) \
	CLASS(_name, scope)(args); \
	if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;

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

[Fabio M. De Francesco]

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;
	} 

I added a similar example in a comment in cleanup.h.

> 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco
> > <fabio.maria.de.francesco@linux.intel.com> ---
> > 
> >  drivers/cxl/core/region.c | 17 +++++------------
> >  include/linux/cleanup.h   | 13 +++++++++++++
> >  2 files changed, 18 insertions(+), 12 deletions(-)
> > 
> > [...] 
> >
> > diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
> > index c2d09bc4f976..fc850e61a47e 100644
> > --- a/include/linux/cleanup.h
> > +++ b/include/linux/cleanup.h
> > @@ -134,6 +134,15 @@ static inline class_##_name##_t
> > class_##_name##ext##_constructor(_init_args) \> 
> >   *	an anonymous instance of the (guard) class, not recommended for
> >   *	conditional locks.
> >   *
> > 
> > + * cond_guard(_name, args...):
> > + * 	for conditional locks like mutex_trylock() or
> > down_read_interruptible(). + * 	It expects a block for handling 
errors
> > like in the following example: + *
> > + * 	cond_guard(rwsem_read_intr, &my_sem) {
> > + * 		printk(KERN_NOTICE "Try failure in work0()\n");
> > + * 		return -EINTR;
> > + * 	}
> > + *
> > 
> >   * scoped_guard (name, args...) { }:
> >   *	similar to CLASS(name, scope)(args), except the variable (with the
> >   *	explicit name 'scope') is declard in a for-loop such that its scope 
is
> > 
> > @@ -165,6 +174,10 @@ static inline class_##_name##_t
> > class_##_name##ext##_constructor(_init_args) \> 
> >  #define __guard_ptr(_name) class_##_name##_lock_ptr
> > 
> > +#define cond_guard(_name, args...) \
> > +	CLASS(_name, scope)(args); \
> > +	if (!__guard_ptr(_name)(&scope))
> 
> This needs to protect against being used within another if () block.
> Imagine a case of:
> 
>     if (...) {
>     	cond_guard(...);
> 	    <statement>
>     } else if (...)
> 
> ...does that "else if" belong to the first "if ()" or the hidden one
> inside the macro?

Good question.

> You can steal the embedded "if ()" trick from scoped_cond_guard() and do
> something like (untested):
> 
> #define cond_guard(_name, _fail, args...) \
> 	CLASS(_name, scope)(args); \
> 	if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;

I think this may work, but...

Again, with this interface there is no means to handle multi-line error paths. 
I wanted an interface sufficiently flexible to handle logging + return -EINTR, 
and possibly more lines to undo something.

Can we have two macros, this for multi-line error paths, and one other, like 
you suggested, for an immediate 'return -EINTR'?

Thanks,

Fabio

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

[Fabio M. De Francesco]

On Tuesday, 30 January 2024 18:02:09 CET Dan Williams wrote:
> Fabio M. De Francesco wrote:

[skip}

> > 
> > @@ -165,6 +174,10 @@ static inline class_##_name##_t
> > class_##_name##ext##_constructor(_init_args) \> 
> >  #define __guard_ptr(_name) class_##_name##_lock_ptr
> > 
> > +#define cond_guard(_name, args...) \
> > +	CLASS(_name, scope)(args); \
> > +	if (!__guard_ptr(_name)(&scope))
> 
> This needs to protect against being used within another if () block.
> Imagine a case of:
> 
>     if (...) {
>     	cond_guard(...);
> 	    <statement>
>     } else if (...)

Could it be made clear in the documentation that cond_guard() shouldn't be 
misused as you showed above? 

Actually, I don't know how effective the documentation can be in avoiding 
incorrect use of cond_guard().

Fabio
 
> ...does that "else if" belong to the first "if ()" or the hidden one
> inside the macro?
> 
> You can steal the embedded "if ()" trick from scoped_cond_guard() and do
> something like (untested):
> 
> #define cond_guard(_name, _fail, args...) \
> 	CLASS(_name, scope)(args); \
> 	if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;

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

[Dan Williams]

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;
    }

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

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

[Ira Weiny]

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

I sympathize with the hidden "if" being confusing but there is already
precedent in the current *_guard macros.  So I'd like to know if Peter has
an opinion.

Ira

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

[Dan Williams]

Ira Weiny wrote:
> 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...
> 
> I sympathize with the hidden "if" being confusing but there is already
> precedent in the current *_guard macros.  So I'd like to know if Peter has
> an opinion.

What are you asking specifically? The current scoped_cond_guard()
already properly encapsulates the "if ()" and takes an "_fail" so why
wouldn't cond_guard() also safely encpsulate an "if ()" and take an
"_fail" statement argument?

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

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > 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...
> > 
> > I sympathize with the hidden "if" being confusing but there is already
> > precedent in the current *_guard macros.  So I'd like to know if Peter has
> > an opinion.
> 
> What are you asking specifically? The current scoped_cond_guard()
> already properly encapsulates the "if ()" and takes an "_fail" so why
> wouldn't cond_guard() also safely encpsulate an "if ()" and take an
> "_fail" statement argument?

Maybe I misunderstood you.  I thought you were advocating that the 'if'
would not be encapsulated.  And I was wondering if Peter had an opinion.

But if you are agreeing with the direction of this patch regarding the if
then ignore me.

Ira

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

[Dan Williams]

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> > > 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...
> > > 
> > > I sympathize with the hidden "if" being confusing but there is already
> > > precedent in the current *_guard macros.  So I'd like to know if Peter has
> > > an opinion.
> > 
> > What are you asking specifically? The current scoped_cond_guard()
> > already properly encapsulates the "if ()" and takes an "_fail" so why
> > wouldn't cond_guard() also safely encpsulate an "if ()" and take an
> > "_fail" statement argument?
> 
> Maybe I misunderstood you.  I thought you were advocating that the 'if'
> would not be encapsulated.  And I was wondering if Peter had an opinion.
> 

Last I sent to Fabio was this:

>> You can steal the embedded "if ()" trick from scoped_cond_guard() and do
>> something like (untested):
>> 
>> #define cond_guard(_name, _fail, args...) \
>>         CLASS(_name, scope)(args); \
>>         if (!__guard_ptr(_name)(&scope)) _fail; else /* pass */;


> But if you are agreeing with the direction of this patch regarding the if
> then ignore me.

I disagree with the proposal that the caller needs to understand that
the macro leaves a dangling "if ()". I am ok with aligning with
scoped_cond_guard() where the caller can assume that the "_fail"
statement has been executed with no "if ()" sequence to terminate.

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.