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

[Vladimir Sementsov-Ogievskiy]

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.

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

> +                }
>                   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.

>                   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..


--
Best regards,
Vladimir