|
From: | supriya kannery |
Subject: | Re: [Qemu-devel] [v9 Patch 5/6]Qemu: Framework for reopening images safely |
Date: | Tue, 22 Nov 2011 15:54:38 +0530 |
User-agent: | Thunderbird 2.0.0.14 (X11/20080501) |
Stefan Hajnoczi wrote:
On Mon, Nov 21, 2011 at 12:13 PM, supriya kannery <address@hidden> wrote:Stefan Hajnoczi wrote:On Fri, Nov 11, 2011 at 6:48 AM, Supriya Kannery <address@hidden> wrote:@@ -708,17 +731,31 @@ int bdrv_reopen(BlockDriverState *bs, in qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name); return ret; } - open_flags = bs->open_flags; - bdrv_close(bs); - ret = bdrv_open(bs, bs->filename, bdrv_flags, drv); - if (ret < 0) { - /* Reopen failed. Try to open with original flags */ - qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); - ret = bdrv_open(bs, bs->filename, open_flags, drv); + /* Use driver specific reopen() if available */ + if (drv->bdrv_reopen_prepare) {This seems weird to me because we're saying a driver may have drv->bdrv_reopen_prepare == NULL but the public bdrv_reopen_prepare() function doesn't check and return -ENOTSUP.If drv->bdrv_reopen_prepare == NULL , then we are not calling the publick bdrv_reopen_prepare at all. Unless we later call public bdrv_reopen_prepare from elsewhere without checking drv->bdrv_reopen_prepare, a check for -ENOTSUP inside the public one is not needed right? Also, we are handling reopening for even those drivers which don't have its own bdrv_reopen_prepare defined, by taking the "else" control path. So condition for reporting "ENOTSUP" shouldn't come up as of now. Please let me know your thoughts.How does VMDK implement its prepare/commit/abort? It needs to use the "public" bdrv_reopen_prepare() function on its image files.
bdrv_reopen() is the public interface which gets called by any of the image formats.
So VMDK or any image format has to call bdrv_reopen which decides to call driver specific prepare/commit/abort or simply close and reopen the file.
BTW I think the bdrv_reopen_*() functions should go in block_int.h and not block.h. They are visible to the block layer but not public to the rest of QEMU, which must use the bdrv_reopen() interface only. I think what's really missing is a way to tie this all together. You have posted raw format and raw-posix protocol patches. But we need to cover image formats, where VMDK is the multi-file special case and qcow2/qed/etc are simpler but also need to be supported. Right now anything but raw-posix is still closing and reopening. By adding support for image formats I think you'll find the right way to structure this code.
Since only bdrv_reopen() is public, it is declared in block.h and structure of
code done in similar way how bdrv_open() is done. The else part in bdrv_reopen() will handle reopen requests for images other than raw for now (simply close and reopen). -thanks, Supriya
Stefan
[Prev in Thread] | Current Thread | [Next in Thread] |