Re: [PATCH] block: report errno when flock fcntl fails

[Kevin Wolf]

Am 16.12.2020 um 14:06 hat David Edmondson geschrieben:
> On Wednesday, 2020-12-16 at 12:57:46 +01, Kevin Wolf wrote:
> 
> > Am 16.12.2020 um 12:38 hat David Edmondson geschrieben:
> >> On Wednesday, 2020-12-16 at 12:29:40 +01, Kevin Wolf wrote:
> >> 
> >> > Am 15.12.2020 um 20:01 hat David Edmondson geschrieben:
> >> >> When a call to fcntl(2) for the purpose of manipulating file locks
> >> >> fails, report the error returned by fcntl.
> >> >> 
> >> >> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
> >> >
> >> > Is appending "Resource temporarily unavailable" in the common case (a
> >> > file locked by another process) actually an improvement for the message
> >> > or more confusing? It would be good to mention the motivation for the
> >> > change in the commit message.
> >> 
> >> Distinguishing between the common case and others is at least a start
> >> when trying to figure out why it failed. We have potentially useful
> >> information to assist in diagnosis, why throw it away?
> >
> > I agree in principle, just when I saw the result, it felt more confusing
> > than helpful. Maybe the best option would be to include the errno only
> > if it's different from EAGAIN? Then the qemu-iotests reference output
> > would probably stay unchanged, too.
> 
> This is a pretty low-level error report even today - unless you
> understand the implementation then it doesn't shout "the file is being
> shared".

Hm, certainly true for raw_apply_lock_bytes(), which shouldn't normally
fail, so I agree that we don't need to simplify messages there.

>From raw_check_lock_bytes(), you get messages like 'Failed to get
"write" lock', which I think is fairly obvious? I'm not saying that it's
perfect and couldn't be improved, of course, but it doesn't feel
horrible.

> Given that, I don't see any value in eliding the EAGAIN message, as I
> have to know that the lack of the errno string means that it was EAGAIN,
> meaning that another process holds a lock.

When you debug an error, you'll look at the code anyway. And a normal
user won't know what EAGAIN or "Resource temporarily unavailable" means
even if it's displayed. Temporarily sounds more like it will go away by
itself, not that I have to shut down a different process first.

I wonder if anyone else has an opinion on this? If people think that
displaying it is preferable or it doesn't matter much, we can add it
despite my concerns.

Kevin