Re: [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Max Reitz]

On 16.11.18 15:02, Max Reitz wrote:
> On 16.11.18 14:58, Alberto Garcia wrote:
>> On Fri 16 Nov 2018 02:34:25 PM CET, Max Reitz wrote:
>>> To me that looks like a problem in the general reopen code.
>>> raw_reopen_prepare() is called and succeeds.  Then
>>> bdrv_reopen_prepare() notices the option wasn't handled and therefore
>>> fails.  bdrv_reopen_multiple() thus doesn't set bs_entry->prepared to
>>> true, which means raw_reopen_abort() won't be called.
>>>
>>> We should always call either BlockDriver.bdrv_reopen_commit() or
>>> BlockDriver.bdrv_reopen_abort() when BlockDriver.bdrv_reopen_prepare()
>>> succeeded.
>>
>> So you mean getting rid of BlockReopenQueueEntry.prepared altogether?
> 
> I mean just calling .bdrv_reopen_abort() in bdrv_reopen_prepare() if
> there was an error after .bdrv_reopen_prepare() succeeded.
> 
> I have a patch, I'm just trying to think of a useful test...

To elaborate, I didn't want to use your test for two reasons.  First, it
seemed generally too dependant on what file-posix's implementation does.
 Second, I couldn't get it to work in any shorter form (which seemed
weird) and why would it even print something about consistent_read?
That seemed weird, too.

I would've thought that just any case where bdrv_reopen_prepare() fails
after a successful .bdrv_reopen_prepare() could trigger the issue, but
that's not the case.

This is because the file locks are not applied to the new FD just by
duping it, they are only applied in raw_reopen_commit().  So if you
prepare without abort without any commit before, the problem disappears.

So there had to be something wrong in raw_reopen_commit(), too.  It
turns out that this is indeed in the above commit, it should pass
"s->locked_shared_perm" instead of "~s->locked_shared_perm" to
raw_apply_lock_bytes().

Max

signature.asc
Description: OpenPGP digital signature