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

[Fam Zheng]

On Mon, Jul 23, 2018 at 10:37 PM Max Reitz <address@hidden> wrote:
>
> On 2018-07-23 03:56, Fam Zheng wrote:
> > 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.
>
> 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