| Message ID | 20111011031159.9587.57559.sendpatchset@skannery.in.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <supriyak@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(); > + 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.) > +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 > + */ > + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); > + 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. > + } > + } > + } > return ret; > } > > Index: qemu/block_int.h > =================================================================== > --- qemu.orig/block_int.h > +++ qemu/block_int.h > @@ -55,6 +55,13 @@ struct BlockDriver { > int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); > int (*bdrv_probe_device)(const char *filename); > int (*bdrv_open)(BlockDriverState *bs, int flags); > + > + /* For handling image reopen for split or non-split files */ > + int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs, > + int flags); > + int (*bdrv_reopen_commit)(BDRVReopenState *rs); > + int (*bdrv_reopen_abort)(BDRVReopenState *rs); > + > int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags); > int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors); > @@ -207,6 +214,14 @@ struct BlockDriverState { > void *private; > }; > > +struct BDRVReopenState { > + BlockDriverState *bs; > + int reopen_flags; > + > + /* For raw-posix */ > + int reopen_fd; > +}; > + > struct BlockDriverAIOCB { > AIOPool *pool; > BlockDriverState *bs; > Index: qemu/qemu-common.h > =================================================================== > --- qemu.orig/qemu-common.h > +++ qemu/qemu-common.h > @@ -228,6 +228,7 @@ typedef struct NICInfo NICInfo; > typedef struct HCIInfo HCIInfo; > typedef struct AudioState AudioState; > typedef struct BlockDriverState BlockDriverState; > +typedef struct BDRVReopenState BDRVReopenState; > typedef struct DriveInfo DriveInfo; > typedef struct DisplayState DisplayState; > typedef struct DisplayChangeListener DisplayChangeListener; > Index: qemu/block/raw.c > =================================================================== > --- qemu.orig/block/raw.c > +++ qemu/block/raw.c > @@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs > return 0; > } > > +static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags) > +{ > + int ret = bdrv_reopen(bs->file, flags); > + if (!ret) { > + bs->open_flags = bs->file->open_flags; > + } > + return ret; > +} > + > 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. Kevin
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().
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 Kannery<supriyak@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 >
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
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
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 + */ + qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename); + ret = bdrv_open(bs, bs->filename, open_flags, drv); + if (ret < 0) { + /* + * Reopen with orig and modified flags failed + */ + abort(); + } + } + } return ret; } Index: qemu/block_int.h =================================================================== --- qemu.orig/block_int.h +++ qemu/block_int.h @@ -55,6 +55,13 @@ struct BlockDriver { int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename); int (*bdrv_probe_device)(const char *filename); int (*bdrv_open)(BlockDriverState *bs, int flags); + + /* For handling image reopen for split or non-split files */ + int (*bdrv_reopen_prepare)(BlockDriverState *bs, BDRVReopenState **rs, + int flags); + int (*bdrv_reopen_commit)(BDRVReopenState *rs); + int (*bdrv_reopen_abort)(BDRVReopenState *rs); + int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags); int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); @@ -207,6 +214,14 @@ struct BlockDriverState { void *private; }; +struct BDRVReopenState { + BlockDriverState *bs; + int reopen_flags; + + /* For raw-posix */ + int reopen_fd; +}; + struct BlockDriverAIOCB { AIOPool *pool; BlockDriverState *bs; Index: qemu/qemu-common.h =================================================================== --- qemu.orig/qemu-common.h +++ qemu/qemu-common.h @@ -228,6 +228,7 @@ typedef struct NICInfo NICInfo; typedef struct HCIInfo HCIInfo; typedef struct AudioState AudioState; typedef struct BlockDriverState BlockDriverState; +typedef struct BDRVReopenState BDRVReopenState; typedef struct DriveInfo DriveInfo; typedef struct DisplayState DisplayState; typedef struct DisplayChangeListener DisplayChangeListener; Index: qemu/block/raw.c =================================================================== --- qemu.orig/block/raw.c +++ qemu/block/raw.c @@ -9,6 +9,15 @@ static int raw_open(BlockDriverState *bs return 0; } +static int raw_reopen(BlockDriverState *bs, BDRVReopenState **prs, int flags) +{ + int ret = bdrv_reopen(bs->file, flags); + if (!ret) { + bs->open_flags = bs->file->open_flags; + } + return ret; +} + 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,
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 <supriyak@linux.vnet.ibm.com> --- block.c | 46 ++++++++++++++++++++++++++++++++-------- block/raw-posix.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ block_int.h | 15 +++++++++++++ 3 files changed, 114 insertions(+), 9 deletions(-)