When preparing for the removal of GNULIB commit 18dca21a32e9 removed the
unneeded O_DIRECTORY, but unfortunately started opening the directory for
writing which fails every time for a directory.  There is also no need for that
as flock() works on O_RDONLY file descriptor as well, even for LOCK_EX.

https://bugzilla.redhat.com/1852741

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virresctrl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 33044da3a1ef..f6da13645ae3 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -456,7 +456,7 @@ VIR_ONCE_GLOBAL_INIT(virResctrl);
 static int
 virResctrlLockWrite(void)
 {
-    int fd = open(SYSFS_RESCTRL_PATH, O_RDWR | O_CLOEXEC);
+    int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
 
     if (fd < 0) {
         virReportSystemError(errno, "%s", _("Cannot open resctrl"));
-- 
2.27.0

On a Thursday in 2020, Martin Kletzander wrote:
>When preparing for the removal of GNULIB commit 18dca21a32e9 removed the
>unneeded O_DIRECTORY, but unfortunately started opening the directory for
>writing which fails every time for a directory.  There is also no need for that
>as flock() works on O_RDONLY file descriptor as well, even for LOCK_EX.
>
>https://bugzilla.redhat.com/1852741
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/util/virresctrl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

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@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

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@redhat.com>
>

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 ;-)

On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote:
> On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
> > 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.

Either

  virFileFlockExclusive(fd)
  virFileFlockShared(fd)

or

  virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE)
  virFileFlock(fd, VIR_FILE_FLOCK_SHARED)

would work. I like the latter better because it's closer to the
original flock(), which it ultimately acts as a very thin wrapper
around of. I'm actually unclear why we'd have the last argument in
the first place: personally I'd just use

  virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK)

and keep things as unambiguous as they can be.

This is all bikeshedding, of course: what actually matters is making
that lock exclusive once again :)

-- 
Andrea Bolognani / Red Hat / Virtualization

On 7/9/20 6:30 PM, Andrea Bolognani wrote:
> On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote:
>> On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
>>> 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.
> 
> Either
> 
>    virFileFlockExclusive(fd)
>    virFileFlockShared(fd)
> 
> or
> 
>    virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE)
>    virFileFlock(fd, VIR_FILE_FLOCK_SHARED)
> 
> would work. I like the latter better because it's closer to the
> original flock(), which it ultimately acts as a very thin wrapper
> around of. I'm actually unclear why we'd have the last argument in
> the first place: personally I'd just use
> 
>    virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK)
> 
> and keep things as unambiguous as they can be.
> 
> This is all bikeshedding, of course: what actually matters is making
> that lock exclusive once again :)
> 

Just realized that for exclusive (aka write) lock, the FD must be opened 
for writing (working on patches for the following report [1] and been 
experimenting a bit and that's what I'm seeing).

1: https://www.redhat.com/archives/libvir-list/2020-July/msg00451.html

Michal

On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote:
>On 7/9/20 6:30 PM, Andrea Bolognani wrote:
>> On Thu, 2020-07-09 at 13:44 +0200, Martin Kletzander wrote:
>>> On Thu, Jul 09, 2020 at 12:50:38PM +0200, Andrea Bolognani wrote:
>>>> 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.
>>
>> Either
>>
>>    virFileFlockExclusive(fd)
>>    virFileFlockShared(fd)
>>
>> or
>>
>>    virFileFlock(fd, VIR_FILE_FLOCK_EXCLUSIVE)
>>    virFileFlock(fd, VIR_FILE_FLOCK_SHARED)
>>
>> would work. I like the latter better because it's closer to the
>> original flock(), which it ultimately acts as a very thin wrapper
>> around of. I'm actually unclear why we'd have the last argument in
>> the first place: personally I'd just use
>>
>>    virFileFlock(fd, VIR_FILE_FLOCK_UNLOCK)
>>
>> and keep things as unambiguous as they can be.
>>
>> This is all bikeshedding, of course: what actually matters is making
>> that lock exclusive once again :)
>>
>
>Just realized that for exclusive (aka write) lock, the FD must be opened
>for writing (working on patches for the following report [1] and been
>experimenting a bit and that's what I'm seeing).
>

Good point, but luckily not related to flock(2).

>1: https://www.redhat.com/archives/libvir-list/2020-July/msg00451.html
>
>Michal
>

On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote:
> On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote:
> > On 7/9/20 6:30 PM, Andrea Bolognani wrote:
> > > This is all bikeshedding, of course: what actually matters is making
> > > that lock exclusive once again :)
> > 
> > Just realized that for exclusive (aka write) lock, the FD must be opened
> > for writing (working on patches for the following report [1] and been
> > experimenting a bit and that's what I'm seeing).
> 
> Good point, but luckily not related to flock(2).

That seems to be the case: according to flock(2),

  A shared or exclusive lock can be placed on a file regardless
  of the mode in which the file was opened.

Michal, does that sound reasonable to you?

-- 
Andrea Bolognani / Red Hat / Virtualization

On 7/10/20 3:39 PM, Andrea Bolognani wrote:
> On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote:
>> On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote:
>>> On 7/9/20 6:30 PM, Andrea Bolognani wrote:
>>>> This is all bikeshedding, of course: what actually matters is making
>>>> that lock exclusive once again :)
>>>
>>> Just realized that for exclusive (aka write) lock, the FD must be opened
>>> for writing (working on patches for the following report [1] and been
>>> experimenting a bit and that's what I'm seeing).
>>
>> Good point, but luckily not related to flock(2).
> 
> That seems to be the case: according to flock(2),
> 
>    A shared or exclusive lock can be placed on a file regardless
>    of the mode in which the file was opened.
> 
> Michal, does that sound reasonable to you?
> 

D'oh! of course this is another case of file locking exemptions. The 
patches I sent earlier today fix code around virFileLock() which is 
fcntl() which is POSIX locking. flock(2) is BSD lock which may or may 
not be implementedusing POSIX locks.

So I think we're okay on that front.
Alternatively, we may switch to OFD (F_OFD_SETLK from fcntl(2)) and 
experience proper file locking. Those are Linux only (but so is resctrl).

Michal

On Fri, Jul 10, 2020 at 04:00:13PM +0200, Michal Privoznik wrote:
>On 7/10/20 3:39 PM, Andrea Bolognani wrote:
>> On Fri, 2020-07-10 at 15:13 +0200, Martin Kletzander wrote:
>>> On Fri, Jul 10, 2020 at 10:47:22AM +0200, Michal Privoznik wrote:
>>>> On 7/9/20 6:30 PM, Andrea Bolognani wrote:
>>>>> This is all bikeshedding, of course: what actually matters is making
>>>>> that lock exclusive once again :)
>>>>
>>>> Just realized that for exclusive (aka write) lock, the FD must be opened
>>>> for writing (working on patches for the following report [1] and been
>>>> experimenting a bit and that's what I'm seeing).
>>>
>>> Good point, but luckily not related to flock(2).
>>
>> That seems to be the case: according to flock(2),
>>
>>    A shared or exclusive lock can be placed on a file regardless
>>    of the mode in which the file was opened.
>>
>> Michal, does that sound reasonable to you?
>>
>
>D'oh! of course this is another case of file locking exemptions. The
>patches I sent earlier today fix code around virFileLock() which is
>fcntl() which is POSIX locking. flock(2) is BSD lock which may or may
>not be implementedusing POSIX locks.
>
>So I think we're okay on that front.
>Alternatively, we may switch to OFD (F_OFD_SETLK from fcntl(2)) and
>experience proper file locking. Those are Linux only (but so is resctrl).
>

Unfortunately it is not stated anywhere that it would have to stay Linux-only
and moreover the kernel recommendation is to use flock(2) which means there are
other applications that will use flock(2) which means we cannot really switch to
any other lock unless guaranteed that it is mutually exclusive with the current
one.  But it would be nice ;-)