On my Fedora 28, /dev/null is locked by some other process (couldn't
inspect it due to the current lslocks limitation), so iotests 226 fails
with some unexpected image locking errors because it uses qemu-io to
open it.

Actually it's safe to not use any lock on /dev/null or /dev/zero.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 60af4b3d51..8bf034108a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         s->use_lock = false;
         break;
     case ON_OFF_AUTO_AUTO:
-        s->use_lock = qemu_has_ofd_lock();
+        if (!strcmp(filename, "/dev/null") ||
+                   !strcmp(filename, "/dev/zero")) {
+            s->use_lock = false;
+        } else {
+            s->use_lock = qemu_has_ofd_lock();
+        }
         break;
     default:
         abort();
-- 
2.17.1

On 07/18/2018 11:41 PM, Fam Zheng wrote:
> On my Fedora 28, /dev/null is locked by some other process (couldn't
> inspect it due to the current lslocks limitation), so iotests 226 fails
> with some unexpected image locking errors because it uses qemu-io to
> open it.
> 
> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 60af4b3d51..8bf034108a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          s->use_lock = false;
>          break;
>      case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        if (!strcmp(filename, "/dev/null") ||
> +                   !strcmp(filename, "/dev/zero")) {
> +            s->use_lock = false;
> +        } else {
> +            s->use_lock = qemu_has_ofd_lock();
> +        }
>          break;
>      default:
>          abort();
> 

I apparently caused a lot of failures with 226, oops!

Should we instead modify the test in this case to not attempt to take a
lock on a device we know cannot meaningfully store state, or is it your
preference to attempt to maintain such a list in the raw driver itself?

I guess we never want QEMU to try to lock things like /dev/zero, but I
don't know if there are more such pseudo-devices we should never try to
lock and if so, what common property unifies them such that we don't
have to maintain a list.

On Thu, 07/19 13:57, John Snow wrote:
> Should we instead modify the test in this case to not attempt to take a
> lock on a device we know cannot meaningfully store state, or is it your
> preference to attempt to maintain such a list in the raw driver itself?
> 
> I guess we never want QEMU to try to lock things like /dev/zero, but I
> don't know if there are more such pseudo-devices we should never try to
> lock and if so, what common property unifies them such that we don't
> have to maintain a list.

They are 0 sized anyway so I only expect them to be used in test cases like the
one we're fixing. So this really doesn't matter.  An exceptional one would be
/dev/nullb* but that is not used in real world either.

I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
are missing), this patch is just trying to make writing test code easier.

Fam

On 07/20/2018 03:24 AM, Fam Zheng wrote:
> On Thu, 07/19 13:57, John Snow wrote:
>> Should we instead modify the test in this case to not attempt to take a
>> lock on a device we know cannot meaningfully store state, or is it your
>> preference to attempt to maintain such a list in the raw driver itself?
>>
>> I guess we never want QEMU to try to lock things like /dev/zero, but I
>> don't know if there are more such pseudo-devices we should never try to
>> lock and if so, what common property unifies them such that we don't
>> have to maintain a list.
> 
> They are 0 sized anyway so I only expect them to be used in test cases like the
> one we're fixing. So this really doesn't matter.  An exceptional one would be
> /dev/nullb* but that is not used in real world either.

I'm not familiar with /dev/nullb* - is that a typo?

$ ll /dev/nullb*
ls: cannot access '/dev/nullb*': No such file or directory


> 
> I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
> are missing), this patch is just trying to make writing test code easier.

/dev/urandom is also a character device, and also fits in my heuristic 
that we likely only care about locking of block devices.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Sat, Jul 21, 2018 at 12:35 AM Eric Blake <eblake@redhat.com> wrote:
>
> On 07/20/2018 03:24 AM, Fam Zheng wrote:
> I'm not familiar with /dev/nullb* - is that a typo?
>
> $ ll /dev/nullb*
> ls: cannot access '/dev/nullb*': No such file or directory

You probably have figured out already but just in case: it's kernel
null_blk mod.

Fam

On 07/20/2018 04:24 AM, Fam Zheng wrote:
> On Thu, 07/19 13:57, John Snow wrote:
>> Should we instead modify the test in this case to not attempt to take a
>> lock on a device we know cannot meaningfully store state, or is it your
>> preference to attempt to maintain such a list in the raw driver itself?
>>
>> I guess we never want QEMU to try to lock things like /dev/zero, but I
>> don't know if there are more such pseudo-devices we should never try to
>> lock and if so, what common property unifies them such that we don't
>> have to maintain a list.
> 
> They are 0 sized anyway so I only expect them to be used in test cases like the
> one we're fixing. So this really doesn't matter.  An exceptional one would be
> /dev/nullb* but that is not used in real world either.
> 

I agree in general; I'm questioning somewhat if we ought to just patch
the test instead of the driver, given that we can't really be consistent
or accurate about when we decide not to take the lock in the driver;
unless we use something like Eric's suggestion (we don't take locks on
char devices, maybe?)

> I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
> are missing), this patch is just trying to make writing test code easier.
> 

Yeah, it's the little quality-of-life band-aid that I'm wondering if we
ought to stick in here. Granted, this fixes a test that's broken, so...

Reviewed-by: John Snow <jsnow@redhat.com>

I'll let Kevin decide if we ought to patch it nicer than this.

--js

> Fam
> 

On 07/19/2018 12:57 PM, John Snow wrote:
> 
> Should we instead modify the test in this case to not attempt to take a
> lock on a device we know cannot meaningfully store state, or is it your
> preference to attempt to maintain such a list in the raw driver itself?
> 
> I guess we never want QEMU to try to lock things like /dev/zero, but I
> don't know if there are more such pseudo-devices we should never try to
> lock and if so, what common property unifies them such that we don't
> have to maintain a list.

One potential common property: /dev/zero and /dev/null are character 
devices, rather than block devices.  Character devices in general don't 
make much sense for holding stateful images, so it may be as simple as 
gating our locking based on fstat() distinguishing which type of file we 
are accessing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 2018-07-19 05:41, Fam Zheng wrote:
> On my Fedora 28, /dev/null is locked by some other process (couldn't
> inspect it due to the current lslocks limitation), so iotests 226 fails
> with some unexpected image locking errors because it uses qemu-io to
> open it.
> 
> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 60af4b3d51..8bf034108a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          s->use_lock = false;
>          break;
>      case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        if (!strcmp(filename, "/dev/null") ||
> +                   !strcmp(filename, "/dev/zero")) {

I’m not sure I like a strcmp() based on filename (though it should work
for all of the cases where someone would want to specify either of those
for a qemu block device).  Isn’t there some specific major/minor number
we can use to check for these two files?  Or are those Linux-specific?

I could also imagine just not locking any host_device, but since people
do use qcow2 immediately on block devices, maybe we do want to lock them.

Finally, if really all you are trying to do is to make test code easier,
then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
be too hard.

Max

> +            s->use_lock = false;
> +        } else {
> +            s->use_lock = qemu_has_ofd_lock();
> +        }
>          break;
>      default:
>          abort();
> 

On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-19 05:41, Fam Zheng wrote:
> > On my Fedora 28, /dev/null is locked by some other process (couldn't
> > inspect it due to the current lslocks limitation), so iotests 226 fails
> > with some unexpected image locking errors because it uses qemu-io to
> > open it.
> >
> > Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 60af4b3d51..8bf034108a 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >          s->use_lock = false;
> >          break;
> >      case ON_OFF_AUTO_AUTO:
> > -        s->use_lock = qemu_has_ofd_lock();
> > +        if (!strcmp(filename, "/dev/null") ||
> > +                   !strcmp(filename, "/dev/zero")) {
>
> I’m not sure I like a strcmp() based on filename (though it should work
> for all of the cases where someone would want to specify either of those
> for a qemu block device).  Isn’t there some specific major/minor number
> we can use to check for these two files?  Or are those Linux-specific?

Yeah, I guess major/minor numbers are Linux-specific.

>
> I could also imagine just not locking any host_device, but since people
> do use qcow2 immediately on block devices, maybe we do want to lock them.

That's right.

>
> Finally, if really all you are trying to do is to make test code easier,
> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> be too hard.

The tricky thing is in remembering to do that for future test cases.
My machine seems to be somehow different than John's so that my 226
cannot lock /dev/null, but I'm not sure that is the case for other
releases, distros or system instances.

Fam

>
> Max
>
> > +            s->use_lock = false;
> > +        } else {
> > +            s->use_lock = qemu_has_ofd_lock();
> > +        }
> >          break;
> >      default:
> >          abort();
> >
>
>

On 2018-07-22 04:37, Fam Zheng wrote:
> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 2018-07-19 05:41, Fam Zheng wrote:
>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
>>> inspect it due to the current lslocks limitation), so iotests 226 fails
>>> with some unexpected image locking errors because it uses qemu-io to
>>> open it.
>>>
>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  block/file-posix.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 60af4b3d51..8bf034108a 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>          s->use_lock = false;
>>>          break;
>>>      case ON_OFF_AUTO_AUTO:
>>> -        s->use_lock = qemu_has_ofd_lock();
>>> +        if (!strcmp(filename, "/dev/null") ||
>>> +                   !strcmp(filename, "/dev/zero")) {
>>
>> I’m not sure I like a strcmp() based on filename (though it should work
>> for all of the cases where someone would want to specify either of those
>> for a qemu block device).  Isn’t there some specific major/minor number
>> we can use to check for these two files?  Or are those Linux-specific?
> 
> Yeah, I guess major/minor numbers are Linux-specific.
> 
>>
>> I could also imagine just not locking any host_device, but since people
>> do use qcow2 immediately on block devices, maybe we do want to lock them.
> 
> That's right.
> 
>>
>> Finally, if really all you are trying to do is to make test code easier,
>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
>> be too hard.
> 
> The tricky thing is in remembering to do that for future test cases.
> My machine seems to be somehow different than John's so that my 226
> cannot lock /dev/null, but I'm not sure that is the case for other
> releases, distros or system instances.

Usually we don’t need to use /dev/null, though, because null-co:// is
better suited for most purposes.  This is a very specific test of host
devices.

Maybe we should start a doc file with common good practices about
writing iotests?

Max

On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-22 04:37, Fam Zheng wrote:
> > On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> On 2018-07-19 05:41, Fam Zheng wrote:
> >>> On my Fedora 28, /dev/null is locked by some other process (couldn't
> >>> inspect it due to the current lslocks limitation), so iotests 226 fails
> >>> with some unexpected image locking errors because it uses qemu-io to
> >>> open it.
> >>>
> >>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >>>
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>> ---
> >>>  block/file-posix.c | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>> index 60af4b3d51..8bf034108a 100644
> >>> --- a/block/file-posix.c
> >>> +++ b/block/file-posix.c
> >>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >>>          s->use_lock = false;
> >>>          break;
> >>>      case ON_OFF_AUTO_AUTO:
> >>> -        s->use_lock = qemu_has_ofd_lock();
> >>> +        if (!strcmp(filename, "/dev/null") ||
> >>> +                   !strcmp(filename, "/dev/zero")) {
> >>
> >> I’m not sure I like a strcmp() based on filename (though it should work
> >> for all of the cases where someone would want to specify either of those
> >> for a qemu block device).  Isn’t there some specific major/minor number
> >> we can use to check for these two files?  Or are those Linux-specific?
> >
> > Yeah, I guess major/minor numbers are Linux-specific.
> >
> >>
> >> I could also imagine just not locking any host_device, but since people
> >> do use qcow2 immediately on block devices, maybe we do want to lock them.
> >
> > That's right.
> >
> >>
> >> Finally, if really all you are trying to do is to make test code easier,
> >> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> >> be too hard.
> >
> > The tricky thing is in remembering to do that for future test cases.
> > My machine seems to be somehow different than John's so that my 226
> > cannot lock /dev/null, but I'm not sure that is the case for other
> > releases, distros or system instances.
>
> Usually we don’t need to use /dev/null, though, because null-co:// is
> better suited for most purposes.  This is a very specific test of host
> devices.
>
> Maybe we should start a doc file with common good practices about
> writing iotests?

Yes, mentioning using pseudo devices in docs/devel/testing.rst would
probably be a good idea. So is my understanding right that you prefer
fixing the test case and discard this patch? If so I'll send another
version together with the doc update.

Fam

>
> Max
>

On 2018-07-23 03:56, Fam Zheng wrote:
> On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 2018-07-22 04:37, Fam Zheng wrote:
>>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>>>>
>>>> On 2018-07-19 05:41, Fam Zheng wrote:
>>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
>>>>> inspect it due to the current lslocks limitation), so iotests 226 fails
>>>>> with some unexpected image locking errors because it uses qemu-io to
>>>>> open it.
>>>>>
>>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
>>>>>
>>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>>> ---
>>>>>  block/file-posix.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>>> index 60af4b3d51..8bf034108a 100644
>>>>> --- a/block/file-posix.c
>>>>> +++ b/block/file-posix.c
>>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>>>          s->use_lock = false;
>>>>>          break;
>>>>>      case ON_OFF_AUTO_AUTO:
>>>>> -        s->use_lock = qemu_has_ofd_lock();
>>>>> +        if (!strcmp(filename, "/dev/null") ||
>>>>> +                   !strcmp(filename, "/dev/zero")) {
>>>>
>>>> I’m not sure I like a strcmp() based on filename (though it should work
>>>> for all of the cases where someone would want to specify either of those
>>>> for a qemu block device).  Isn’t there some specific major/minor number
>>>> we can use to check for these two files?  Or are those Linux-specific?
>>>
>>> Yeah, I guess major/minor numbers are Linux-specific.
>>>
>>>>
>>>> I could also imagine just not locking any host_device, but since people
>>>> do use qcow2 immediately on block devices, maybe we do want to lock them.
>>>
>>> That's right.
>>>
>>>>
>>>> Finally, if really all you are trying to do is to make test code easier,
>>>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
>>>> be too hard.
>>>
>>> The tricky thing is in remembering to do that for future test cases.
>>> My machine seems to be somehow different than John's so that my 226
>>> cannot lock /dev/null, but I'm not sure that is the case for other
>>> releases, distros or system instances.
>>
>> Usually we don’t need to use /dev/null, though, because null-co:// is
>> better suited for most purposes.  This is a very specific test of host
>> devices.
>>
>> Maybe we should start a doc file with common good practices about
>> writing iotests?
> 
> Yes, mentioning using pseudo devices in docs/devel/testing.rst would
> probably be a good idea. So is my understanding right that you prefer
> fixing the test case and discard this patch? If so I'll send another
> version together with the doc update.

I personally would prefer fixing the test and not modifying the code,
yes.  But I'm aware that it is a personal opinion.

I think another alternative would be to not lock character devices, as I
don't suppose anyone is going to use them for anything serious.

Max

On Mon, Jul 23, 2018 at 10:37 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-23 03:56, Fam Zheng wrote:
> > On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> On 2018-07-22 04:37, Fam Zheng wrote:
> >>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
> >>>>
> >>>> On 2018-07-19 05:41, Fam Zheng wrote:
> >>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
> >>>>> inspect it due to the current lslocks limitation), so iotests 226 fails
> >>>>> with some unexpected image locking errors because it uses qemu-io to
> >>>>> open it.
> >>>>>
> >>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >>>>>
> >>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>>>> ---
> >>>>>  block/file-posix.c | 7 ++++++-
> >>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>>>> index 60af4b3d51..8bf034108a 100644
> >>>>> --- a/block/file-posix.c
> >>>>> +++ b/block/file-posix.c
> >>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >>>>>          s->use_lock = false;
> >>>>>          break;
> >>>>>      case ON_OFF_AUTO_AUTO:
> >>>>> -        s->use_lock = qemu_has_ofd_lock();
> >>>>> +        if (!strcmp(filename, "/dev/null") ||
> >>>>> +                   !strcmp(filename, "/dev/zero")) {
> >>>>
> >>>> I’m not sure I like a strcmp() based on filename (though it should work
> >>>> for all of the cases where someone would want to specify either of those
> >>>> for a qemu block device).  Isn’t there some specific major/minor number
> >>>> we can use to check for these two files?  Or are those Linux-specific?
> >>>
> >>> Yeah, I guess major/minor numbers are Linux-specific.
> >>>
> >>>>
> >>>> I could also imagine just not locking any host_device, but since people
> >>>> do use qcow2 immediately on block devices, maybe we do want to lock them.
> >>>
> >>> That's right.
> >>>
> >>>>
> >>>> Finally, if really all you are trying to do is to make test code easier,
> >>>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> >>>> be too hard.
> >>>
> >>> The tricky thing is in remembering to do that for future test cases.
> >>> My machine seems to be somehow different than John's so that my 226
> >>> cannot lock /dev/null, but I'm not sure that is the case for other
> >>> releases, distros or system instances.
> >>
> >> Usually we don’t need to use /dev/null, though, because null-co:// is
> >> better suited for most purposes.  This is a very specific test of host
> >> devices.
> >>
> >> Maybe we should start a doc file with common good practices about
> >> writing iotests?
> >
> > Yes, mentioning using pseudo devices in docs/devel/testing.rst would
> > probably be a good idea. So is my understanding right that you prefer
> > fixing the test case and discard this patch? If so I'll send another
> > version together with the doc update.
>
> I personally would prefer fixing the test and not modifying the code,
> yes.  But I'm aware that it is a personal opinion.

Sure, and you are the maintainer, so why not follow what you think. :)

Fam