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.

While at it, also change the only caller so that it acquires an exclusive lock
as it should've been all the time.  This was caused because the function was
ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
misused in commit 657ddeff2313 and since then the lock being taken was shared
rather than exclusive.

Signed-off-by: Martin Kletzander <mkletzan@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;
+    }
+
+    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..784a8d43bb2f 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_EXCLUSIVE) < 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

On Fri, 2020-07-10 at 15:20 +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.
> 
> While at it, also change the only caller so that it acquires an exclusive lock
> as it should've been all the time.  This was caused because the function was
> ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
> misused in commit 657ddeff2313 and since then the lock being taken was shared
> rather than exclusive.

I don't think you should do both things in the same patch: please
change the virFileFlock() interface, with no semantic changes, in one
patch, and fix virResctrlLockWrite() in a separate one.

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

This switch() statement is missing

  default:
      virReportEnumRangeError(virFileFlockOperation, op);

And I thought you liked Rust?!? :D

> +++ 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_EXCLUSIVE) < 0) {

As mentioned above, use VIR_FILE_FLOCK_SHARED here and then change it
to VIR_FILE_FLOCK_EXCLUSIVE in a separate patch.


With the default case added and the semantic-altering changes
removed,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

On Fri, Jul 10, 2020 at 03:50:07PM +0200, Andrea Bolognani wrote:
>On Fri, 2020-07-10 at 15:20 +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.
>>
>> While at it, also change the only caller so that it acquires an exclusive lock
>> as it should've been all the time.  This was caused because the function was
>> ambiguous since it was created in commit 5a0a5f7fb5f5, and due to that it was
>> misused in commit 657ddeff2313 and since then the lock being taken was shared
>> rather than exclusive.
>
>I don't think you should do both things in the same patch: please
>change the virFileFlock() interface, with no semantic changes, in one
>patch, and fix virResctrlLockWrite() in a separate one.
>
>> +    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;
>> +    }
>
>This switch() statement is missing
>
>  default:
>      virReportEnumRangeError(virFileFlockOperation, op);
>

Did that exist back in the old days when I was sending patches??? =D Anyway
yeah, it's an enum, not a sum type.

>And I thought you liked Rust?!? :D
>

That match {} would not have to have the default, that's one of the reasons I
did not add it in the first place ;)

>> +++ 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_EXCLUSIVE) < 0) {
>
>As mentioned above, use VIR_FILE_FLOCK_SHARED here and then change it
>to VIR_FILE_FLOCK_EXCLUSIVE in a separate patch.
>

I agree with that in all other patches.  In this one, where resctrl is (and most
probably will forever stay) the only user of virFileFlock()... I'll just do it
without agreeing.

>
>With the default case added and the semantic-altering changes
>removed,
>
>  Reviewed-by: Andrea Bolognani <abologna@redhat.com>
>
>-- 
>Andrea Bolognani / Red Hat / Virtualization
>