Am 30.10.2011 11:35, schrieb Supriya Kannery: > raw-posix driver changes for bdrv_reopen_xx functions to > safely reopen image files. Reopening of image files while > changing hostcache dynamically, is handled here. > > Signed-off-by: Supriya Kannery <supri...@linux.vnet.ibm.com> > > Index: qemu/block/raw.c > =================================================================== > --- qemu.orig/block/raw.c > +++ qemu/block/raw.c > @@ -9,6 +9,24 @@ static int raw_open(BlockDriverState *bs > return 0; > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + return bdrv_reopen_prepare(bs->file, prs, flags); > +} > + > +static void raw_reopen_commit(BlockDriverState *bs, BDRVReopenState *rs, > + int flags) > +{ > + bdrv_reopen_commit(bs->file, rs, flags); > + bs->open_flags = bs->file->open_flags;
I think it should be bs->open_flags = flags, even if the underlying driver masks something away (which it shouldn't do in the first place, but using flags here is clearer). Also I'm wondering if updating bs->open_flags isn't common to all formats, so could we move it into the block.c function? > +} > + > +static void raw_reopen_abort(BlockDriverState *bs, BDRVReopenState *rs) > +{ > + bdrv_reopen_abort(bs->file, rs); > +} > + > static int coroutine_fn raw_co_readv(BlockDriverState *bs, int64_t > sector_num, > int nb_sectors, QEMUIOVector *qiov) > { > @@ -107,7 +125,10 @@ static BlockDriver bdrv_raw = { > > /* It's really 0, but we need to make g_malloc() happy */ > .instance_size = 1, > - > + .bdrv_reopen_prepare > + = raw_reopen_prepare, > + .bdrv_reopen_commit = raw_reopen_commit, > + .bdrv_reopen_abort = raw_reopen_abort, > .bdrv_open = raw_open, > .bdrv_close = raw_close, Mostly a matter of taste, but I would prefer open/close to stay first and having bdrv_reopen_* only after them. > > Index: qemu/block/raw-posix.c > =================================================================== > --- qemu.orig/block/raw-posix.c > +++ qemu/block/raw-posix.c > @@ -279,6 +279,60 @@ static int raw_open(BlockDriverState *bs > return raw_open_common(bs, filename, flags, 0); > } > > +static int raw_reopen_prepare(BlockDriverState *bs, BDRVReopenState **prs, > + int flags) > +{ > + BDRVReopenState *raw_rs = g_malloc0(sizeof(BDRVReopenState)); > + BDRVRawState *s = bs->opaque; > + > + raw_rs->bs = bs; > + raw_rs->reopen_flags = s->open_flags; > + raw_rs->reopen_fd = -1; > + *prs = raw_rs; > + int ret = 0; > + > + /* If only O_DIRECT to be toggled, use fcntl */ > + if (!((bs->open_flags & ~BDRV_O_NOCACHE) ^ > + (flags & ~BDRV_O_NOCACHE))) { Wouldn't it be more readable like this? /* Use fcntl if all affected flags can be changes this way */ fcntl_flags = BDRV_O_NOCACHE; if ((bs->open_flags & ~fcntl_flags) == (flags & ~fcntl_flags)) { ... > + raw_rs->reopen_fd = dup(s->fd); > + if (raw_rs->reopen_fd <= 0) { > + return -1; > + } > + if ((flags & BDRV_O_NOCACHE)) { > + raw_rs->reopen_flags |= O_DIRECT; > + } else { > + raw_rs->reopen_flags &= ~O_DIRECT; > + } > + ret = fcntl_setfl(raw_rs->reopen_fd, raw_rs->reopen_flags); > + } I think there should be an else branch that returns an error. Currently requests involving anything but BDRV_O_NOCACHE are completely ignored, but still success is returned. > + > + /* TBD: Handle O_DSYNC and other flags */ > + > + return ret; > +} Kevin