Am 05.08.2011 11:29, schrieb Stefan Hajnoczi: > On Fri, Aug 5, 2011 at 10:07 AM, Kevin Wolf <kw...@redhat.com> wrote: >> Am 05.08.2011 10:40, schrieb Stefan Hajnoczi: >>> We've discussed safe methods for reopening image files (e.g. useful for >>> changing the hostcache parameter). The problem is that closing the file >>> first >>> and then opening it again exposes us to the error case where the open fails. >>> At that point we cannot get to the file anymore and our options are to >>> terminate QEMU, pause the VM, or offline the block device. >>> >>> This window of vulnerability can be eliminated by keeping the file >>> descriptor >>> around and falling back to it should the open fail. >>> >>> The challenge for the file descriptor approach is that image formats, like >>> VMDK, can span multiple files. Therefore the solution is not as simple as >>> stashing a single file descriptor and reopening from it. >> >> So far I agree. The rest I believe is wrong because you can't assume >> that every backend uses file descriptors. The qemu block layer is based >> on BlockDriverStates, not fds. They are a concept that should be hidden >> in raw-posix. >> >> I think something like this could do: >> >> struct BDRVReopenState { >> BlockDriverState *bs; >> /* can be extended by block drivers */ >> }; >> >> .bdrv_reopen(BlockDriverState *bs, BDRVReopenState **reopen_state, int >> flags); >> .bdrv_reopen_commit(BDRVReopenState *reopen_state); >> .bdrv_reopen_abort(BDRVReopenState *reopen_state); >> >> raw-posix would store the old file descriptor in its reopen_state. On >> commit, it closes the old descriptors, on abort it reverts to the old >> one and closes the newly opened one. >> >> Makes things a bit more complicated than the simple bdrv_reopen I had in >> mind before, but it allows VMDK to get an all-or-nothing semantics. > > Can you show how bdrv_reopen() would use these new interfaces? I'm > not 100% clear on the idea.
Well, you wouldn't only call bdrv_reopen, but also either bdrv_reopen_commit/abort (for the top-level caller we can have a wrapper function that does both, but that's syntactic sugar). For example we would have: int vmdk_reopen() { *((VMDKReopenState**) rs) = malloc(); foreach (extent in s->extents) { ret = bdrv_reopen(extent->file, &extent->reopen_state) if (ret < 0) goto fail; } return 0; fail: foreach (extent in rs->already_reopened) { bdrv_reopen_abort(extent->reopen_state); } return ret; } void vmdk_reopen_commit() { foreach (extent in s->extents) { bdrv_reopen_commit(extent->reopen_state); } free(rs); } void vmdk_reopen_abort() { foreach (extent in s->extents) { bdrv_reopen_abort(extent->reopen_state); } free(rs); } The top-level caller, which isn't a block driver, but just wants to have an image reopened, will do something like this (as I said, this should probably be a wrapper function in block.c): BDRVReopenState *rs; ret = bdrv_reopen(bs, &rs); if (ret < 0) { goto fail; } bdrv_reopen_commit(rs); Kevin