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

[Martin Kletzander]

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 at redhat.com>
>
>-- 
>Andrea Bolognani / Red Hat / Virtualization
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200710/b99aa6ba/attachment-0001.sig>