On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote:
> static int
> virResctrlLockWrite(void)
> {
> - int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
> + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
I got curious, so I did some digging.
The commit message for 18dca21a32e9 says
The O_DIRECTORY flag causes open() to return an error if the
filename is a directory. There's no obvious reason why resctrl
needs to use this [...]
but open(2) claims
O_DIRECTORY
If pathname is not a directory, cause the open to fail.
instead, so using O_DIRECTORY in the first place doesn't seem at all
unreasonable: the resctrl mount path *is* going to be a directory. I
guess we don't need to be too paranoid about it, though, so I don't
think removing the flag is a big issue.
Now, when it comes to opening R/O or R/W, I agree that using the
latter it was not the right choice and it altered the behavior, but
I assume the decision was influenced by the name of the function,
virResctrlLockWrite().
I looked back through the file's history to see whether that name was
justified by virResctrlLockRead() existing at some point, but that
doesn't seem to be the case. I guess the name was inspired by
virRWLockWrite()? But since there's no "read" equivalent, it seems
that the simpler virResctrlLock() would have worked just as fine. But
I digress.
The more interesting thing I noticed is that in 657ddeff2313, the
locking call was changed from
flock(fd, LOCK_EX)
to
virFileFlock(fd, true, true)
and given the documentation for virFileFlock()
/**
* virFileFlock:
* @fd: file descriptor to call flock on
* @lock: true for lock, false for unlock
* @shared: true if shared, false for exclusive, ignored if
* `@lock == false`
*
* This is just a simple wrapper around flock(2) that errors out
* on unsupported platforms.
*
* The lock will be released when @fd is closed or this function
* is called with `@lock == false`.
*
* Returns 0 on success, -1 otherwise (with errno set)
*/
int virFileFlock(int fd, bool lock, bool shared)
it seems to me that this change entirely flipped the semantics of
the lock.
tl;dr
Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>
for this patch, but we probably need to change the second argument
of the virFileFlock() call and maybe consider renaming the function.
--
Andrea Bolognani / Red Hat / Virtualization
