Re: [PATCH 4/6] log: Add _perror() logging function variants

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On Wed, Jun 19, 2024 at 10:25:17AM +0200, Stefano Brivio wrote:
> On Wed, 19 Jun 2024 12:11:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 18, 2024 at 08:02:16AM +0200, Stefano Brivio wrote:
> > > On Tue, 18 Jun 2024 10:46:36 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Mon, Jun 17, 2024 at 02:03:17PM +0200, Stefano Brivio wrote:  
> > > > > In many places, we have direct perror() calls, which completely bypass
> > > > > logging functions and log files.
> > > > > 
> > > > > They are definitely convenient: offer similar convenience with
> > > > > _perror() logging variants, so that we can drop those direct perror()
> > > > > calls.
> > > > > 
> > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>    
> > > > 
> > > > Hm, for anything bigger than like a screenful of code, I generally
> > > > find an explicit message with strerror(errno) more useful than
> > > > perror() or equivalents, but I guess if you think these are useful.  
> > > 
> > > Okay, yes, it probably makes sense to have more descriptive messages as
> > > you suggest in the comment to 5/6, but even then, we still have a lot
> > > of cases like this one (from 6/6):
> > > 
> > > -		warn("lseek() failed on /proc/net file: %s", strerror(errno));
> > > +		warn_perror("lseek() failed on /proc/net file");
> > > 
> > > where these _perror() variants make for tidier code, I find, regardless
> > > of the error message itself.  
> > 
> > Eh, I mildly prefer the first variant.  It is slightly longer, but
> > makes it very clear where the strerror piece is going to appear in the
> > context of the whole message.  It's not a strong preference, though.
> 
> Well, it depends. If you're used to perror() it's obvious where the
> error description will appear, and it's actually faster for me to read
> something called "_perror" than %s plus the argument. Plus we can save
> a few lines like that and substantially improve readability in some
> cases:
> 
>  		if (prctl(PR_CAPBSET_DROP, i, 0, 0, 0) &&
>  		    errno != EINVAL && errno != EPERM)
> -			die("Couldn't drop cap %i from bounding set: %s",
> -			    i, strerror(errno));
> +			die_perror("Couldn't drop cap %i from bounding set", i);

Eh, ok.  You more or less convinced me.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]