Re: [PATCH] resctrl: Do not open directory for writing

On Thu, 2020-07-09 at 11:40 +0200, Martin Kletzander wrote: 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(a)redhat.com&gt; 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