[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
Date: Mon, 23 Jul 2018 16:37:48 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

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.


Attachment: signature.asc
Description: OpenPGP digital signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]