Re: [PATCH] util: Rework virFileFlock() to be unambiguous
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization