linux-kernel - Re: [RFC PATCH v2] cleanup: Add cond

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <65b938c1ad435_5cc6f294eb@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Tue, 30 Jan 2024 09:58:25 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: "Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>,
	<linux-kernel@...r.kernel.org>, Dan Williams <dan.j.williams@...el.com>
CC: Peter Zijlstra <peterz@...radead.org>, Dan Williams
	<dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...el.com>
Subject: Re: [RFC PATCH v2] cleanup: Add cond_guard() to conditional guards

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.