[PATCH] util: Rework virFileFlock() to be unambiguous

[Andrea Bolognani]

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 at redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization