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

[Max Reitz]

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.

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

signature.asc
Description: OpenPGP digital signature