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

[David Edmondson <dme@dme.org>]

On Wednesday, 2021-01-13 at 13:26:48 +03, Vladimir Sementsov-Ogievskiy wrote:

> 12.01.2021 18:27, David Edmondson wrote:
>> When a call to fcntl(2) for the purpose of manipulating file locks
>> fails with an error other than EAGAIN or EACCES, report the error
>> returned by fcntl.
>> 
>> EAGAIN or EACCES are elided as they are considered to be common
>> failures, indicating that a conflicting lock is held by another
>> process.
>> 
>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>> v3:
>> - Remove the now unnecessary updates to the test framework (Max).
>> - Elide the error detail for EAGAIN or EACCES when locking (Kevin,
>>    sort-of Max).
>> - Philippe and Vladimir sent Reviewed-by, but things have changed
>>    noticeably, so I didn't add them (dme).
>> 
>> v4:
>> - Really, really remove the unnecessary updates to the test framework.
>> 
>>   block/file-posix.c | 52 +++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 42 insertions(+), 10 deletions(-)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 00cdaaa2d4..c5142f7ffa 100644
>
> Hmm, not it looks like too much code duplication. Maybe, we can add a helper macro, like
>
> #define raw_lock_error_setg_errno(errp, os_error, fmt, ...) \
>    do {
>      if (err == EAGAIN || err == EACCES) {
>        error_setg((errp), (fmt), ## __VA_ARGS__);
>      } else {
>        error_setg_errno((errp), (os_error), (fmt), ## __VA_ARGS__);
>      }
>    } while (0)
>
> We can't make a helper function instead, as error_setg_errno is already a macro and it wants to use __LINE__..
>
> But I think that macro is better than duplication anyway.

Okay.

>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -836,7 +836,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>>           if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
>>               ret = qemu_lock_fd(fd, off, 1, false);
>>               if (ret) {
>> -                error_setg(errp, "Failed to lock byte %d", off);
>> +                int err = -ret;
>> +
>> +                if (err == EAGAIN || err == EACCES) {
>> +                    error_setg(errp, "Failed to lock byte %d", off);
>> +                } else {
>> +                    error_setg_errno(errp, err, "Failed to lock byte %d", off);
>> +                }
>>                   return ret;
>>               } else if (s) {
>>                   s->locked_perm |= bit;
>> @@ -844,7 +850,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>>           } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {
>>               ret = qemu_unlock_fd(fd, off, 1);
>>               if (ret) {
>> -                error_setg(errp, "Failed to unlock byte %d", off);
>> +                int err = -ret;
>> +
>> +                if (err == EAGAIN || err == EACCES) {
>> +                    error_setg(errp, "Failed to lock byte %d", off);
>
> s/lock/unlock
>
>> +                } else {
>> +                    error_setg_errno(errp, err, "Failed to lock byte %d", off);
>
> and here.
>
> Which proves, that code duplication is a bad idea in general :)

Ouch. Will fix.

>
>> +                }
>>                   return ret;
>>               } else if (s) {
>>                   s->locked_perm &= ~bit;
>> @@ -857,7 +869,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>>           if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
>>               ret = qemu_lock_fd(fd, off, 1, false);
>>               if (ret) {
>> -                error_setg(errp, "Failed to lock byte %d", off);
>> +                int err = -ret;
>> +
>> +                if (err == EAGAIN || err == EACCES) {
>> +                    error_setg(errp, "Failed to lock byte %d", off);
>> +                } else {
>> +                    error_setg_errno(errp, err, "Failed to lock byte %d", off);
>> +                }
>>                   return ret;
>>               } else if (s) {
>>                   s->locked_shared_perm |= bit;
>> @@ -866,7 +884,7 @@ static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
>>                      !(shared_perm_lock_bits & bit)) {
>>               ret = qemu_unlock_fd(fd, off, 1);
>>               if (ret) {
>> -                error_setg(errp, "Failed to unlock byte %d", off);
>> +                error_setg_errno(errp, -ret, "Failed to unlock byte %d", off);
>
> Don't know, why same logic is not applied here.. Probably I've missed from the previous discussion. Anyway, if it is intentional exclusion, not bad to mention it in commit message.

From the documentation, it didn't seem to make sense that the unlock
case would generate EAGAIN or EACCES for "someone is already holding the
lock", so neither should be elided.

I can add a note in the commit message.

>>                   return ret;
>>               } else if (s) {
>>                   s->locked_shared_perm &= ~bit;
>> @@ -890,9 +908,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm, uint64_t shared_perm,
>>               ret = qemu_lock_fd_test(fd, off, 1, true);
>>               if (ret) {
>>                   char *perm_name = bdrv_perm_names(p);
>> -                error_setg(errp,
>> -                           "Failed to get \"%s\" lock",
>> -                           perm_name);
>> +                int err = -ret;
>> +
>> +                if (err == EAGAIN || err == EACCES) {
>> +                    error_setg(errp, "Failed to get \"%s\" lock",
>> +                               perm_name);
>
> fit in one line
>
>> +                } else {
>> +                    error_setg_errno(errp, err,
>> +                                     "Failed to get \"%s\" lock",
>> +                                     perm_name);
>
> fit in two lines..
>
>> +                }
>>                   g_free(perm_name);
>>                   return ret;
>>               }
>> @@ -905,9 +930,16 @@ static int raw_check_lock_bytes(int fd, uint64_t perm, uint64_t shared_perm,
>>               ret = qemu_lock_fd_test(fd, off, 1, true);
>>               if (ret) {
>>                   char *perm_name = bdrv_perm_names(p);
>> -                error_setg(errp,
>> -                           "Failed to get shared \"%s\" lock",
>> -                           perm_name);
>> +                int err = -ret;
>> +
>> +                if (err == EAGAIN || err == EACCES) {
>> +                    error_setg(errp, "Failed to get shared \"%s\" lock",
>> +                               perm_name);
>> +                } else {
>> +                    error_setg_errno(errp, err,
>> +                                     "Failed to get shared \"%s\" lock",
>> +                                     perm_name);
>> +                }
>>                   g_free(perm_name);
>>                   return ret;
>>               }
>> 
>
> also, I don't see much benefit in creating additional "err" variable instead of just use ret, but it's a kind of taste..

Okay.

dme.
-- 
Tonight I'm gonna bury that horse in the ground.