Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/8] block: Ignore loosening perm restrictions failures

[Max Reitz]

On 22.05.19 20:24, Eric Blake wrote:
> On 5/22/19 12:03 PM, Max Reitz wrote:
>> Hi,
>>
>> This series is mainly a fix for
>> https://bugzilla.redhat.com/show_bug.cgi?id=1703793.  The problem
>> described there is that mirroring to a gluster volume, then switching
>> off the volume makes qemu crash.  There are two problems here:
>>
>> (1) file-posix reopens the FD all the time because it thinks the FD it
>>     has is RDONLY.  It actually isn’t after the first reopen, we just
>>     forgot to change the internal flags.  That’s what patch 1 is for.
>>
>> (2) Even then, when mirror completes, it drops its write permission on
>>     the FD.  This requires a reopen, which will fail if the volume is
>>     down.  Mirror doesn’t expect that.  Nobody ever expects that
>>     dropping permissions can fail, and rightfully so because that’s what
>>     I think we have generally agreed on.
>>     Therefore, the block layer should hide this error.  This is what the
>>     last two patches are for.
>>
>> The penultimate patch adds two assertions: bdrv_replace_child() (for the
>> old BDS) and bdrv_inactivate_recurse() assume they only ever drop
>> assertions.  This is now substantiated by these new assertions.
>> It turns out that this assumption was just plain wrong.  Patches 3 to 5
>> make it right.
>>
>>
>> v3:
>> - Received no reply to my “Hm, warnings break 'make check', so maybe we
>>   should just keep quiet if loosening restrictions fails?” question, so
>>   I assume silence means agreement.  Changed patch 7 accordingly.
>>
> 
> I don't know if there is an easy way to warn for normal users, but
> silence the warnings if run under test setups to keep 'make check'
> output unchanged (I know we've silenced warnings in the past when we
> detect we are running qtest, but this isn't necessarily the same setup).
>  So not a show-stopper for me.

Hm.  That doesn’t sound too bad.  I don’t think there is an easy way to
silence the warning in qemu, but we might be able to just modify the test.

But I don’t even know whether the warnings are even useful or whether
they would just confuse users more than anything.  So far, every case I
know where loosening restrictions failed was because the file is just
gone completely.  The only purpose of a warning is to show the user that
qemu might have locks on the file that it doesn’t need, so they will
know what’s up if they try to open the file in another qemu instance in
a way that should normally work but suddenly doesn’t.  But if the file’s
just gone, you can’t open it in another qemu, so I don’t even know
whether there’s actually any point in warning.

Max

signature.asc
Description: OpenPGP digital signature