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. Fam