On 25.10.2016 15:43, Fam Zheng wrote:
> On Tue, 10/25 15:28, Max Reitz wrote:
>> On 25.10.2016 08:31, Fam Zheng wrote:
>>> On Sat, 10/22 01:40, Max Reitz wrote:
>>>> On 30.09.2016 14:09, Fam Zheng wrote:
>>
>> [...]
>>
>>>>> +static int
>>>>> +raw_reopen_upgrade(BDRVReopenState *state,
>>>>> + RawReopenOperation op,
>>>>> + ImageLockMode old_lock,
>>>>> + ImageLockMode new_lock,
>>>>> + Error **errp)
>>>>> +{
>>>>> + BDRVRawReopenState *raw_s = state->opaque;
>>>>> + BDRVRawState *s = state->bs->opaque;
>>>>> + int ret = 0, ret2;
>>>>> +
>>>>> + assert(old_lock == IMAGE_LOCK_MODE_SHARED);
>>>>> + assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
>>>>> + switch (op) {
>>>>> + case RAW_REOPEN_PREPARE:
>>>>> + ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [1]
>>>
>>>>> + if (ret) {
>>>>> + error_setg_errno(errp, -ret, "Failed to lock new fd
>>>>> (shared)");
>>>>> + break;
>>>>> + }
>>>>> + ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
>>>
>>> [2]
>>>
>>>>> + if (ret) {
>>>>> + error_setg_errno(errp, -ret, "Failed to unlock old fd");
>>>>> + goto restore;
>>>>> + }
>>>>> + ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
>>>
>>> [3]
>>>
>>>>> + if (ret) {
>>>>> + error_setg_errno(errp, -ret, "Failed to lock new fd
>>>>> (exclusive)");
>>>>> + goto restore;
>>>>> + }
>>>>> + break;
>>>>> + case RAW_REOPEN_COMMIT:
>>>>> + break;
>>>>> + case RAW_REOPEN_ABORT:
>>>>> + raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>>> + ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [4]
>>>
>>>>> + if (ret) {
>>>>> + error_report("Failed to restore lock on old fd");
>>>>
>>>> If we get here, s->lock_fd is still locked exclusively. The following is
>>>> a very personal opinion, but anyway: I think it would be be better for
>>>> it to be unlocked. If it's locked too strictly, this can really break
>>>> something; but if it's just not locked (while it should be locked in
>>>> shared mode), everything's going to be fine unless the user makes a
>>>> mistake. I think the latter is less bad.
>>>
>>> There are four lock states when we land on this "abort" branch:
>>>
>>> a) Lock "prepare" was not run, some other error happened earlier, so the
>>> lock
>>> aren't changed compared to before the transaction starts:
>>> raw_s->lock_fd is
>>> unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4]
>>> cannot
>>> fail, and even if it does, s->lock_fd is in the correct state.
>>>
>>> b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is
>>> intact, so
>>> we are good.
>>>
>>> c) The raw_lock_fd [2] failed. Again, similar to above.
>>>
>>> d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
>>> s->lock_fd is unlocked. The correct "abort" sequence is downgrade
>>> raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort"
>>> sequence
>>> failed, s->lock_fd is unlocked.
>>
>> OK, you're right, I somehow forgot about the cases where our prepare
>> sequence was either not run at all or failed. But I was thinking about
>> the case where our preparation was successful, but some later
>> preparation in the reopen transaction failed. Then, s->lock_fd should be
>> locked exclusively, no?
>
> Oh I missed to reason about that branch. Here we go:
>
> If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->lock_fd
> is unlocked. In abort we should try to return to the old state (raw_s->lock_fd
> is _not_ exclusively locked and s->lock_fd is shared locked), hence the two
> raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() at [4]
> doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed
> anyway,
> which again matches what you suggested.Oh, right, it's raw_s->lock_fd that's exclusively locked, not s->lock_fd... See? I really can't tell the difference between raw_s and s. :-/ So you're right, but that makes me just all the more skeptical about raw_s... Max
signature.asc
Description: OpenPGP digital signature
