[PATCH v2 1/2] util: Rework virFileFlock() to be unambiguous

[Martin Kletzander]

On Fri, Jul 10, 2020 at 09:45:25PM +0200, Martin Kletzander wrote:
>The boolean parameters for lock/unlock and read/write (shared/exclusive) caused
>a lot of confusion when reading the callers.  The new approach is explicit and
>unambiguous.
>
>Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>Reviewed-by: Andrea Bolognani <abologna at redhat.com>
>---
> src/util/virfile.c    | 27 ++++++++++++++++++---------
> src/util/virfile.h    |  9 ++++++++-
> src/util/virresctrl.c |  4 ++--
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
>diff --git a/src/util/virfile.c b/src/util/virfile.c
>index 213acdbcaa2b..554011eecb25 100644
>--- a/src/util/virfile.c
>+++ b/src/util/virfile.c
>@@ -465,23 +465,33 @@ int virFileUnlock(int fd, off_t start, off_t len)
> /**
>  * virFileFlock:
>  * @fd: file descriptor to call flock on
>- * @lock: true for lock, false for unlock
>- * @shared: true if shared, false for exclusive, ignored if `@lock == false`
>+ * @op: operation to perform
>  *
>  * This is just a simple wrapper around flock(2) that errors out on unsupported
>  * platforms.
>  *
>  * The lock will be released when @fd is closed or this function is called with
>- * `@lock == false`.
>+ * `@op == VIR_FILE_FLOCK_UNLOCK`.
>  *
>  * Returns 0 on success, -1 otherwise (with errno set)
>  */
>-int virFileFlock(int fd, bool lock, bool shared)
>+int virFileFlock(int fd, virFileFlockOperation op)
> {
>-    if (lock)
>-        return flock(fd, shared ? LOCK_SH : LOCK_EX);
>+    int flock_op = -1;
>
>-    return flock(fd, LOCK_UN);
>+    switch (op) {
>+    case VIR_FILE_FLOCK_SHARED:
>+        flock_op = LOCK_SH;
>+        break;
>+    case VIR_FILE_FLOCK_EXCLUSIVE:
>+        flock_op = LOCK_EX;
>+        break;
>+    case VIR_FILE_FLOCK_UNLOCK:
>+        flock_op = LOCK_UN;
>+        break;

Yes, I know I forgot to add the default branch and it complicates matters even
more because it would set an error in a function that just sets errno...  I'll
send a v3.

>+    }
>+
>+    return flock(fd, flock_op);
> }
>
> #else /* WIN32 */
>@@ -505,8 +515,7 @@ int virFileUnlock(int fd G_GNUC_UNUSED,
>
>
> int virFileFlock(int fd G_GNUC_UNUSED,
>-                 bool lock G_GNUC_UNUSED,
>-                 bool shared G_GNUC_UNUSED)
>+                 virFileFlockOperation op G_GNUC_UNUSED)
> {
>     errno = ENOSYS;
>     return -1;
>diff --git a/src/util/virfile.h b/src/util/virfile.h
>index 7a92364a5c9f..04428727fd3b 100644
>--- a/src/util/virfile.h
>+++ b/src/util/virfile.h
>@@ -118,7 +118,14 @@ int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock)
> int virFileUnlock(int fd, off_t start, off_t len)
>     G_GNUC_NO_INLINE;
>
>-int virFileFlock(int fd, bool lock, bool shared);
>+
>+typedef enum {
>+    VIR_FILE_FLOCK_EXCLUSIVE,
>+    VIR_FILE_FLOCK_SHARED,
>+    VIR_FILE_FLOCK_UNLOCK,
>+} virFileFlockOperation;
>+
>+int virFileFlock(int fd, virFileFlockOperation op);
>
> typedef int (*virFileRewriteFunc)(int fd, const void *opaque);
> int virFileRewrite(const char *path,
>diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>index 3563fc560db5..b685b8bb6f7b 100644
>--- a/src/util/virresctrl.c
>+++ b/src/util/virresctrl.c
>@@ -463,7 +463,7 @@ virResctrlLockWrite(void)
>         return -1;
>     }
>
>-    if (virFileFlock(fd, true, true) < 0) {
>+    if (virFileFlock(fd, VIR_FILE_FLOCK_SHARED) < 0) {
>         virReportSystemError(errno, "%s", _("Cannot lock resctrl"));
>         VIR_FORCE_CLOSE(fd);
>         return -1;
>@@ -485,7 +485,7 @@ virResctrlUnlock(int fd)
>         virReportSystemError(errno, "%s", _("Cannot close resctrl"));
>
>         /* Trying to save the already broken */
>-        if (virFileFlock(fd, false, false) < 0)
>+        if (virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK) < 0)
>             virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
>
>         return -1;
>-- 
>2.27.0
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200710/4ac60386/attachment-0001.sig>