Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote: Index: qemu/block.c === --- qemu.orig/block.c +++ qemu/block.c @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in { BlockDriver *drv = bs-drv; int ret = 0, open_flags; +BDRVReopenState *rs; BDRVReopenState *rs = NULL; If the abort path is taken we need to make sure rs has a defined value. Note that the abort path currently doesn't handle rs == NULL and will segfault in raw_reopen_abort().
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
On 10/12/2011 08:25 PM, Kevin Wolf wrote: Am 11.10.2011 05:11, schrieb Supriya Kannery: Struct BDRVReopenState introduced for handling reopen state of images. This can be extended by each of the block drivers to reopen respective image files. Implementation for raw-posix is done here. Signed-off-by: Supriya Kannerysupri...@linux.vnet.ibm.com Maybe it would make sense to split this into two patches, one for the block.c infrastructure and another one for adding the callbacks in drivers. ok..will split in v8. + +/* If only O_DIRECT to be toggled, use fcntl */ +if (!((bs-open_flags ~BDRV_O_NOCACHE) ^ +(flags ~BDRV_O_NOCACHE))) { +raw_rs-reopen_fd = dup(s-fd); +if (raw_rs-reopen_fd= 0) { +return -1; This leaks raw_rs. will handle +} +} + +/* TBD: Handle O_DSYNC and other flags */ +*rs = raw_rs; +return 0; +} + +static int raw_reopen_commit(BDRVReopenState *rs) bdrv_reopen_commit must never fail. Any error that can happen must already be handled in bdrv_reopen_prepare. The commit function should really only do s-fd = rs-reopen_fd (besides cleanup), everything else should already be prepared. will move call to fcntl to bdrv_reopen_prepare. +{ +BlockDriverState *bs = rs-bs; +BDRVRawState *s = bs-opaque; + +if (!rs-reopen_fd) { +return -1; +} + +int ret = fcntl_setfl(rs-reopen_fd, rs-reopen_flags); reopen_flags is BDRV_O_*, not O_*, so it needs to be translated. ok +/* Use driver specific reopen() if available */ +if (drv-bdrv_reopen_prepare) { +ret = drv-bdrv_reopen_prepare(bs,rs, bdrv_flags); if (ret 0) { -/* Reopen failed with orig and modified flags */ -abort(); +goto fail; } -} +if (drv-bdrv_reopen_commit) { +ret = drv-bdrv_reopen_commit(rs); +if (ret 0) { +goto fail; +} +return 0; +} Pull the return 0; out one level. It would be really strange if we turned a successful prepare into reopen_abort just because the driver doesn't need a commit function. (The other consistent way would be to require that if a driver implements one reopen function, it has to implement all three of them. I'm fine either way.) Will give flexibility to drivers, not mandating all the three functions. +ret = bdrv_open(bs, bs-filename, open_flags, drv); +if (ret 0) { +/* + * Reopen with orig and modified flags failed + */ +abort(); Make this bs-drv = NULL, so that trying to access to image will fail, but at least not the whole VM crashes. ok static int raw_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors) { @@ -128,6 +137,8 @@ static BlockDriver bdrv_raw = { .instance_size = 1, .bdrv_open = raw_open, +.bdrv_reopen_prepare += raw_reopen, .bdrv_close = raw_close, .bdrv_read = raw_read, .bdrv_write = raw_write, I think raw must pass through all three functions. Otherwise it could happen that we need to abort, but the image has already been reopened. ok..got it..will have three separate functions to avoid unnecessary dependencies. Got a question.. In raw-posix, the three functions are implemented for file_reopen for now. Should this be extended to hdev, cdrom and floppy? Kevin
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
On 10/14/2011 04:43 PM, Stefan Hajnoczi wrote: On Tue, Oct 11, 2011 at 08:41:59AM +0530, Supriya Kannery wrote: Index: qemu/block.c === --- qemu.orig/block.c +++ qemu/block.c @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in { BlockDriver *drv = bs-drv; int ret = 0, open_flags; +BDRVReopenState *rs; BDRVReopenState *rs = NULL; If the abort path is taken we need to make sure rs has a defined value. Note that the abort path currently doesn't handle rs == NULL and will segfault in raw_reopen_abort(). sure..will check on this. thanks, Supriya
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
Am 14.10.2011 15:42, schrieb Supriya Kannery: +/* Use driver specific reopen() if available */ +if (drv-bdrv_reopen_prepare) { +ret = drv-bdrv_reopen_prepare(bs,rs, bdrv_flags); if (ret 0) { -/* Reopen failed with orig and modified flags */ -abort(); +goto fail; } -} +if (drv-bdrv_reopen_commit) { +ret = drv-bdrv_reopen_commit(rs); +if (ret 0) { +goto fail; +} +return 0; +} Pull the return 0; out one level. It would be really strange if we turned a successful prepare into reopen_abort just because the driver doesn't need a commit function. (The other consistent way would be to require that if a driver implements one reopen function, it has to implement all three of them. I'm fine either way.) Will give flexibility to drivers, not mandating all the three functions. Do we have a use case where it is actually possible to implement less functions without introducing bugs? If yes, let's keep it as it is. Got a question.. In raw-posix, the three functions are implemented for file_reopen for now. Should this be extended to hdev, cdrom and floppy? Yes, that would be good. And I think the same implementation can be used for all of them. Kevin
Re: [Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
Am 11.10.2011 05:11, schrieb Supriya Kannery: Struct BDRVReopenState introduced for handling reopen state of images. This can be extended by each of the block drivers to reopen respective image files. Implementation for raw-posix is done here. Signed-off-by: Supriya Kannery supri...@linux.vnet.ibm.com Maybe it would make sense to split this into two patches, one for the block.c infrastructure and another one for adding the callbacks in drivers. --- block.c | 46 block/raw-posix.c | 62 ++ block_int.h | 15 + 3 files changed, 114 insertions(+), 9 deletions(-) Index: qemu/block/raw-posix.c === --- qemu.orig/block/raw-posix.c +++ qemu/block/raw-posix.c @@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs return raw_open_common(bs, filename, flags, 0); } +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs, + int flags) +{ +BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState)); +BDRVRawState *s = bs-opaque; + +raw_rs-bs = bs; +raw_rs-reopen_flags = flags; +raw_rs-reopen_fd = -1; + +/* If only O_DIRECT to be toggled, use fcntl */ +if (!((bs-open_flags ~BDRV_O_NOCACHE) ^ +(flags ~BDRV_O_NOCACHE))) { +raw_rs-reopen_fd = dup(s-fd); +if (raw_rs-reopen_fd = 0) { +return -1; This leaks raw_rs. +} +} + +/* TBD: Handle O_DSYNC and other flags */ +*rs = raw_rs; +return 0; +} + +static int raw_reopen_commit(BDRVReopenState *rs) bdrv_reopen_commit must never fail. Any error that can happen must already be handled in bdrv_reopen_prepare. The commit function should really only do s-fd = rs-reopen_fd (besides cleanup), everything else should already be prepared. +{ +BlockDriverState *bs = rs-bs; +BDRVRawState *s = bs-opaque; + +if (!rs-reopen_fd) { +return -1; +} + +int ret = fcntl_setfl(rs-reopen_fd, rs-reopen_flags); reopen_flags is BDRV_O_*, not O_*, so it needs to be translated. +if (ret 0) { +return ret; +} + +/* Set new flags, so replace old fd with new one */ +close(s-fd); +s-fd = rs-reopen_fd; +s-open_flags = bs-open_flags = rs-reopen_flags; +g_free(rs); +rs = NULL; Setting to NULL has no effect, rs isn't read afterwards. + +return 0; +} + +static int raw_reopen_abort(BDRVReopenState *rs) This must not fail either, so it can be void, too. +{ +if (rs-reopen_fd != -1) { +close(rs-reopen_fd); +rs-reopen_fd = -1; +rs-reopen_flags = 0; +} +g_free(rs); +rs = NULL; +return 0; +} + /* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE { @@ -896,6 +956,9 @@ static BlockDriver bdrv_file = { .instance_size = sizeof(BDRVRawState), .bdrv_probe = NULL, /* no probe for protocols */ .bdrv_file_open = raw_open, +.bdrv_reopen_prepare = raw_reopen_prepare, +.bdrv_reopen_commit = raw_reopen_commit, +.bdrv_reopen_abort = raw_reopen_abort, .bdrv_read = raw_read, .bdrv_write = raw_write, .bdrv_close = raw_close, @@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = { .instance_size = sizeof(BDRVRawState), .bdrv_probe_device = hdev_probe_device, .bdrv_file_open = hdev_open, +.bdrv_reopen_prepare += raw_reopen_prepare, +.bdrv_reopen_commit = raw_reopen_commit, +.bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, .bdrv_create= hdev_create, .create_options = raw_create_options, Index: qemu/block.c === --- qemu.orig/block.c +++ qemu/block.c @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in { BlockDriver *drv = bs-drv; int ret = 0, open_flags; +BDRVReopenState *rs; /* Quiesce IO for the given block device */ qemu_aio_flush(); @@ -713,20 +714,48 @@ 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) { +ret = drv-bdrv_reopen_prepare(bs, rs, bdrv_flags); if (ret 0) { -/* Reopen failed with orig and modified flags */ -abort(); +
[Qemu-devel] [v7 Patch 5/5]Qemu: New struct 'BDRVReopenState' for image files reopen
Struct BDRVReopenState introduced for handling reopen state of images. This can be extended by each of the block drivers to reopen respective image files. Implementation for raw-posix is done here. Signed-off-by: Supriya Kannery supri...@linux.vnet.ibm.com --- block.c | 46 block/raw-posix.c | 62 ++ block_int.h | 15 + 3 files changed, 114 insertions(+), 9 deletions(-) Index: qemu/block/raw-posix.c === --- qemu.orig/block/raw-posix.c +++ qemu/block/raw-posix.c @@ -279,6 +279,66 @@ static int raw_open(BlockDriverState *bs return raw_open_common(bs, filename, flags, 0); } +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **rs, + int flags) +{ +BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState)); +BDRVRawState *s = bs-opaque; + +raw_rs-bs = bs; +raw_rs-reopen_flags = flags; +raw_rs-reopen_fd = -1; + +/* If only O_DIRECT to be toggled, use fcntl */ +if (!((bs-open_flags ~BDRV_O_NOCACHE) ^ +(flags ~BDRV_O_NOCACHE))) { +raw_rs-reopen_fd = dup(s-fd); +if (raw_rs-reopen_fd = 0) { +return -1; +} +} + +/* TBD: Handle O_DSYNC and other flags */ +*rs = raw_rs; +return 0; +} + +static int raw_reopen_commit(BDRVReopenState *rs) +{ +BlockDriverState *bs = rs-bs; +BDRVRawState *s = bs-opaque; + +if (!rs-reopen_fd) { +return -1; +} + +int ret = fcntl_setfl(rs-reopen_fd, rs-reopen_flags); +if (ret 0) { +return ret; +} + +/* Set new flags, so replace old fd with new one */ +close(s-fd); +s-fd = rs-reopen_fd; +s-open_flags = bs-open_flags = rs-reopen_flags; +g_free(rs); +rs = NULL; + +return 0; +} + +static int raw_reopen_abort(BDRVReopenState *rs) +{ +if (rs-reopen_fd != -1) { +close(rs-reopen_fd); +rs-reopen_fd = -1; +rs-reopen_flags = 0; +} +g_free(rs); +rs = NULL; +return 0; +} + /* XXX: use host sector size if necessary with: #ifdef DIOCGSECTORSIZE { @@ -896,6 +956,9 @@ static BlockDriver bdrv_file = { .instance_size = sizeof(BDRVRawState), .bdrv_probe = NULL, /* no probe for protocols */ .bdrv_file_open = raw_open, +.bdrv_reopen_prepare = raw_reopen_prepare, +.bdrv_reopen_commit = raw_reopen_commit, +.bdrv_reopen_abort = raw_reopen_abort, .bdrv_read = raw_read, .bdrv_write = raw_write, .bdrv_close = raw_close, @@ -1166,6 +1229,10 @@ static BlockDriver bdrv_host_device = { .instance_size = sizeof(BDRVRawState), .bdrv_probe_device = hdev_probe_device, .bdrv_file_open = hdev_open, +.bdrv_reopen_prepare += raw_reopen_prepare, +.bdrv_reopen_commit = raw_reopen_commit, +.bdrv_reopen_abort = raw_reopen_abort, .bdrv_close = raw_close, .bdrv_create= hdev_create, .create_options = raw_create_options, Index: qemu/block.c === --- qemu.orig/block.c +++ qemu/block.c @@ -706,6 +706,7 @@ int bdrv_reopen(BlockDriverState *bs, in { BlockDriver *drv = bs-drv; int ret = 0, open_flags; +BDRVReopenState *rs; /* Quiesce IO for the given block device */ qemu_aio_flush(); @@ -713,20 +714,48 @@ 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) { +ret = drv-bdrv_reopen_prepare(bs, rs, bdrv_flags); if (ret 0) { -/* Reopen failed with orig and modified flags */ -abort(); +goto fail; } -} +if (drv-bdrv_reopen_commit) { +ret = drv-bdrv_reopen_commit(rs); +if (ret 0) { +goto fail; +} +return 0; +} +fail: +if (drv-bdrv_reopen_abort) { +ret = drv-bdrv_reopen_abort(rs); +qerror_report(QERR_REOPEN_FILE_FAILED, bs-filename); +return ret; +} + +} else { + +/* Use reopen procedure common for all drivers */ +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 +*/ +