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@xxxxxxxxxx>
--
Andrea Bolognani / Red Hat / Virtualization
