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@xxxxxxxxxx> -- Andrea Bolognani / Red Hat / Virtualization
Attachment:
signature.asc
Description: PGP signature
