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

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

[David Edmondson]

When a call to fcntl(2) for the purpose of adding 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.

No errors are elided when removing file locks.

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.

v5:
- Use a macro to avoid duplicating the EAGAIN/EACCES suppression
  (Vladimir).
- Fix "lock" -> "unlock" (Vladimir).
- Comment on not eliding errors for the unlock case (Vladimir).

 block/file-posix.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 00cdaaa2d4..11aafa9d82 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -216,6 +216,20 @@ typedef struct RawPosixAIOData {
 static int cdrom_reopen(BlockDriverState *bs);
 #endif
 
+/*
+ * Elide EAGAIN and EACCES details when failing to lock, as this
+ * indicates that the specified file region is already locked by
+ * another process, which is considered a common scenario.
+ */
+#define raw_lock_error_setg_errno(errp, err, fmt, ...)                  \
+    do {                                                                \
+        if ((err) == EAGAIN || (err) == EACCES) {                       \
+            error_setg((errp), (fmt), ## __VA_ARGS__);                  \
+        } else {                                                        \
+            error_setg_errno((errp), (err), (fmt), ## __VA_ARGS__);     \
+        }                                                               \
+    } while (0)
+
 #if defined(__NetBSD__)
 static int raw_normalize_devicepath(const char **filename, Error **errp)
 {
@@ -836,7 +850,8 @@ 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);
+                raw_lock_error_setg_errno(errp, -ret, "Failed to lock byte %d",
+                                          off);
                 return ret;
             } else if (s) {
                 s->locked_perm |= bit;
@@ -844,7 +859,7 @@ 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);
+                error_setg_errno(errp, -ret, "Failed to unlock byte %d", off);
                 return ret;
             } else if (s) {
                 s->locked_perm &= ~bit;
@@ -857,7 +872,8 @@ 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);
+                raw_lock_error_setg_errno(errp, -ret, "Failed to lock byte %d",
+                                          off);
                 return ret;
             } else if (s) {
                 s->locked_shared_perm |= bit;
@@ -866,7 +882,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);
                 return ret;
             } else if (s) {
                 s->locked_shared_perm &= ~bit;
@@ -890,9 +906,10 @@ 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);
+
+                raw_lock_error_setg_errno(errp, -ret,
+                                          "Failed to get \"%s\" lock",
+                                          perm_name);
                 g_free(perm_name);
                 return ret;
             }
@@ -905,9 +922,10 @@ 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);
+
+                raw_lock_error_setg_errno(errp, -ret,
+                                          "Failed to get shared \"%s\" lock",
+                                          perm_name);
                 g_free(perm_name);
                 return ret;
             }
-- 
2.29.2

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

[Vladimir Sementsov-Ogievskiy]

13.01.2021 19:44, David Edmondson wrote:
> When a call to fcntl(2) for the purpose of adding 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.
> 
> No errors are elided when removing file locks.
> 
> 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.
> 
> v5:
> - Use a macro to avoid duplicating the EAGAIN/EACCES suppression
>    (Vladimir).
> - Fix "lock" -> "unlock" (Vladimir).
> - Comment on not eliding errors for the unlock case (Vladimir).

Thanks!

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Max Reitz]

On 13.01.21 17:44, David Edmondson wrote:
> When a call to fcntl(2) for the purpose of adding 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.
> 
> No errors are elided when removing file locks.
> 
> 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.
> 
> v5:
> - Use a macro to avoid duplicating the EAGAIN/EACCES suppression
>    (Vladimir).
> - Fix "lock" -> "unlock" (Vladimir).
> - Comment on not eliding errors for the unlock case (Vladimir).

Thanks!  I’ve applied this patch to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max