On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
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 [...]
I think that was a typo, yes.
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.
The main reason for removing it was that it is not available on other platforms, I think it is linux-only and gnulib was handling other platforms for us. We don't really care if it is a directory or not. The very next read somewhere in that path will fail if it is not a directory. We only need to make sure we use flock(2) as that is the lock used for mutual exclusion on resctrl. Yes, it is Linux-only, just as O_DIRECTORY, and same as resctrl itself. At least to my knowledge. We could also wrap it in an #ifdef clause, but there is no need for it, really. virFileFlock() will currently fail with ENOSYS for non-Linux (or non-__linux__, to be overly specific).
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().
That could've been, although it refers to a write (exclusive) lock.
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.
So, funny[0] story, there was. It was just never committed because i could not use it in the end. The only usage would be for reporting some information in capabilities for example. Since flock(2) does not have lock promotion (and it's always an issue anyway) reading information has to use a write lock if we are going to write something based on the values we read, otherwise the data we would base the settings on could change in the meantime. It's not a proof, but you can see that by looking up virResctrlLockInternal() which used to be the function deduplicating same code between read and write locking.
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.
It should be just Lock, no Write, it's confusing, I agree.
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)
Yes, that was part of a clean-up that did not get in as a whole, IIRC. Further information available only on in-person requests and enough alcohol nearby[1].
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.
Yep, you're right. When you have a lock that has boolean as a parameter I think the boolean should indicate whether the lock is writable, but maybe the proper and most readable solution would be virFileFlockShareable() and virFileFlockExclusive() to avoid any misconception in the future[2]. Given the fact that there are no (and most probably won't be) any other users of flock(2) and given the fact that resctrl is also pretty niche feature, I don't have any preference. Also I feel like after some time I'm a little bit rusty with C and the libvirt codebase (and most importantly the reasoning and decisions) has changed a bit, so I'll leave the decision on how to deal with that on someone else. I'm happy to provide the patches when clear decision is made.
tl;dr Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>
Thanks, I'm pushing this one (let's see if I screw it up or not) and will prepare for the next ones.
for this patch, but we probably need to change the second argument of the virFileFlock() call and maybe consider renaming the function.
[0] not really funny at all
[1] drink responsibly or not at all
[2] well, apart from having a proper sum type and the flock function only
accepting Flock::Exclusive or Flock::Shared for that parameter or something
along those lines ;-)
Attachment:
signature.asc
Description: PGP signature
