[PATCH] resctrl: Do not open directory for writing
abologna at redhat.com
Thu Jul 9 10:50:38 UTC 2020
On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote:
> static int
> - 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
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,
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
The more interesting thing I noticed is that in 657ddeff2313, the
locking call was changed from
virFileFlock(fd, true, true)
and given the documentation for 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
Reviewed-by: Andrea Bolognani <abologna at redhat.com>
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
More information about the libvir-list