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

[Stefano Brivio <sbrivio@redhat.com>]

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

-- 
Stefano