Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically

[Fam Zheng]

On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <address@hidden> wrote:
>
> On 2018-07-22 04:37, Fam Zheng wrote:
> > On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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
>