Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

2012-09-20 Thread Kevin Wolf 
Am 18.09.2012 20:53, schrieb Jeff Cody:
 This is derived from the Supriya Kannery's reopen patches.
 
 This contains the raw-posix driver changes for the bdrv_reopen_*
 functions.  All changes are staged into a temporary scratch buffer
 during the prepare() stage, and copied over to the live structure
 during commit().  Upon abort(), all changes are abandoned, and the
 live structures are unmodified.
 
 The _prepare() will create an extra fd - either by means of a dup,
 if possible, or opening a new fd if not (for instance, access
 control changes).  Upon _commit(), the original fd is closed and
 the new fd is used.  Upon _abort(), the duplicate/new fd is closed.
 
 Signed-off-by: Jeff Cody jc...@redhat.com

 +static int raw_reopen_prepare(BDRVReopenState *state,
 +  BlockReopenQueue *queue, Error **errp)
 +{
 +BDRVRawState *s;
 +BDRVRawReopenState *raw_s;
 +int ret = 0;
 +
 +assert(state != NULL);
 +assert(state-bs != NULL);
 +
 +s = state-bs-opaque;
 +
 +state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
 +raw_s = state-opaque;
 +raw_s-use_aio = s-use_aio;
 +raw_s-aio_ctx = s-aio_ctx;

You can immediately set s-aio_ctx instead of going through
BDRVRawReopenState with it. It seems to be valid to have it present
while use_aio = 0. If it wasn't valid, you'd have to free the context
when reopening without Linux AIO.

 +
 +raw_parse_flags(state-flags, raw_s-open_flags);
 +raw_set_aio(raw_s-aio_ctx, raw_s-use_aio, state-flags);

At least you're consistently omitting the error check. :-)

 +
 +raw_s-fd = -1;
 +
 +int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
 +#ifdef O_NOATIME
 +fcntl_flags |= O_NOATIME;
 +#endif
 +if ((raw_s-open_flags  ~fcntl_flags) == (s-open_flags  
 ~fcntl_flags)) {
 +/* dup the original fd */
 +/* TODO: use qemu fcntl wrapper */

Hm, still not addressed?

Kevin

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

2012-09-20 Thread Jeff Cody 
On 09/20/2012 10:10 AM, Kevin Wolf wrote:
 Am 18.09.2012 20:53, schrieb Jeff Cody:
 This is derived from the Supriya Kannery's reopen patches.

 This contains the raw-posix driver changes for the bdrv_reopen_*
 functions.  All changes are staged into a temporary scratch buffer
 during the prepare() stage, and copied over to the live structure
 during commit().  Upon abort(), all changes are abandoned, and the
 live structures are unmodified.

 The _prepare() will create an extra fd - either by means of a dup,
 if possible, or opening a new fd if not (for instance, access
 control changes).  Upon _commit(), the original fd is closed and
 the new fd is used.  Upon _abort(), the duplicate/new fd is closed.

 Signed-off-by: Jeff Cody jc...@redhat.com
 
 +static int raw_reopen_prepare(BDRVReopenState *state,
 +  BlockReopenQueue *queue, Error **errp)
 +{
 +BDRVRawState *s;
 +BDRVRawReopenState *raw_s;
 +int ret = 0;
 +
 +assert(state != NULL);
 +assert(state-bs != NULL);
 +
 +s = state-bs-opaque;
 +
 +state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
 +raw_s = state-opaque;
 +raw_s-use_aio = s-use_aio;
 +raw_s-aio_ctx = s-aio_ctx;
 
 You can immediately set s-aio_ctx instead of going through
 BDRVRawReopenState with it. It seems to be valid to have it present
 while use_aio = 0. If it wasn't valid, you'd have to free the context
 when reopening without Linux AIO.


Good catch, thanks.

 +
 +raw_parse_flags(state-flags, raw_s-open_flags);
 +raw_set_aio(raw_s-aio_ctx, raw_s-use_aio, state-flags);
 
 At least you're consistently omitting the error check. :-)
 

Thanks, fixed.

I guess I am an optimist at heart :)

 +
 +raw_s-fd = -1;
 +
 +int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
 +#ifdef O_NOATIME
 +fcntl_flags |= O_NOATIME;
 +#endif
 +if ((raw_s-open_flags  ~fcntl_flags) == (s-open_flags  
 ~fcntl_flags)) {
 +/* dup the original fd */
 +/* TODO: use qemu fcntl wrapper */
 
 Hm, still not addressed?
 

No. I mentioned this in the cover letter for v2.  I'd rather see the
qemu fcntl wrapper happen as a separate series, and then come back and
update this, if that is OK.  I'm afraid changes to qemu_open or
qemu_dup_flags would delay getting this series in.

Although Eric is right, I do need to ifdef the F_DUPFD_CLOEXEC.


 Kevin

[Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

2012-09-18 Thread Jeff Cody 
This is derived from the Supriya Kannery's reopen patches.

This contains the raw-posix driver changes for the bdrv_reopen_*
functions.  All changes are staged into a temporary scratch buffer
during the prepare() stage, and copied over to the live structure
during commit().  Upon abort(), all changes are abandoned, and the
live structures are unmodified.

The _prepare() will create an extra fd - either by means of a dup,
if possible, or opening a new fd if not (for instance, access
control changes).  Upon _commit(), the original fd is closed and
the new fd is used.  Upon _abort(), the duplicate/new fd is closed.

Signed-off-by: Jeff Cody jc...@redhat.com
---
 block/raw-posix.c | 99 +++
 1 file changed, 99 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6bf5480..edd1eca 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -138,6 +138,15 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+int fd;
+int open_flags;
+#ifdef CONFIG_LINUX_AIO
+int use_aio;
+void *aio_ctx;
+#endif
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 
@@ -288,6 +297,93 @@ static int raw_open(BlockDriverState *bs, const char 
*filename, int flags)
 return raw_open_common(bs, filename, flags, 0);
 }
 
+static int raw_reopen_prepare(BDRVReopenState *state,
+  BlockReopenQueue *queue, Error **errp)
+{
+BDRVRawState *s;
+BDRVRawReopenState *raw_s;
+int ret = 0;
+
+assert(state != NULL);
+assert(state-bs != NULL);
+
+s = state-bs-opaque;
+
+state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
+raw_s = state-opaque;
+raw_s-use_aio = s-use_aio;
+raw_s-aio_ctx = s-aio_ctx;
+
+raw_parse_flags(state-flags, raw_s-open_flags);
+raw_set_aio(raw_s-aio_ctx, raw_s-use_aio, state-flags);
+
+raw_s-fd = -1;
+
+int fcntl_flags = O_APPEND | O_ASYNC | O_NONBLOCK;
+#ifdef O_NOATIME
+fcntl_flags |= O_NOATIME;
+#endif
+if ((raw_s-open_flags  ~fcntl_flags) == (s-open_flags  ~fcntl_flags)) {
+/* dup the original fd */
+/* TODO: use qemu fcntl wrapper */
+raw_s-fd = fcntl(s-fd, F_DUPFD_CLOEXEC, 0);
+if (raw_s-fd = 0) {
+ret = fcntl_setfl(raw_s-fd, raw_s-open_flags);
+if (ret) {
+qemu_close(raw_s-fd);
+raw_s-fd = -1;
+}
+}
+}
+
+/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
+if (raw_s-fd == -1) {
+assert(!(raw_s-open_flags  O_CREAT));
+raw_s-fd = qemu_open(state-bs-filename, raw_s-open_flags);
+if (raw_s-fd == -1) {
+ret = -1;
+}
+}
+
+return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+BDRVRawReopenState *raw_s = state-opaque;
+BDRVRawState *s = state-bs-opaque;
+
+s-open_flags = raw_s-open_flags;
+
+qemu_close(s-fd);
+s-fd = raw_s-fd;
+s-use_aio = raw_s-use_aio;
+s-aio_ctx = raw_s-aio_ctx;
+
+g_free(state-opaque);
+state-opaque = NULL;
+}
+
+
+static void raw_reopen_abort(BDRVReopenState *state)
+{
+BDRVRawReopenState *raw_s = state-opaque;
+
+ /* nothing to do if NULL, we didn't get far enough */
+if (raw_s == NULL) {
+return;
+}
+
+if (raw_s-fd = 0) {
+qemu_close(raw_s-fd);
+raw_s-fd = -1;
+}
+g_free(state-opaque);
+state-opaque = NULL;
+}
+
+
 /* XXX: use host sector size if necessary with:
 #ifdef DIOCGSECTORSIZE
 {
@@ -738,6 +834,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_close = raw_close,
 .bdrv_create = raw_create,
 .bdrv_co_discard = raw_co_discard,
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

2012-09-18 Thread Eric Blake 
On 09/18/2012 12:53 PM, Jeff Cody wrote:
 This is derived from the Supriya Kannery's reopen patches.
 
 This contains the raw-posix driver changes for the bdrv_reopen_*
 functions.  All changes are staged into a temporary scratch buffer
 during the prepare() stage, and copied over to the live structure
 during commit().  Upon abort(), all changes are abandoned, and the
 live structures are unmodified.
 
 The _prepare() will create an extra fd - either by means of a dup,
 if possible, or opening a new fd if not (for instance, access
 control changes).  Upon _commit(), the original fd is closed and
 the new fd is used.  Upon _abort(), the duplicate/new fd is closed.
 
 Signed-off-by: Jeff Cody jc...@redhat.com
 ---
  block/raw-posix.c | 99 
 +++
  1 file changed, 99 insertions(+)
 
 diff --git a/block/raw-posix.c b/block/raw-posix.c
 index 6bf5480..edd1eca 100644
 --- a/block/raw-posix.c
 +++ b/block/raw-posix.c
 @@ -138,6 +138,15 @@ typedef struct BDRVRawState {
  #endif
  } BDRVRawState;
  
 +typedef struct BDRVRawReopenState {
 +int fd;
 +int open_flags;
 +#ifdef CONFIG_LINUX_AIO
 +int use_aio;
 +void *aio_ctx;
 +#endif

These members are conditional...

 +static int raw_reopen_prepare(BDRVReopenState *state,
 +  BlockReopenQueue *queue, Error **errp)
 +{
 +BDRVRawState *s;
 +BDRVRawReopenState *raw_s;
 +int ret = 0;
 +
 +assert(state != NULL);
 +assert(state-bs != NULL);
 +
 +s = state-bs-opaque;
 +
 +state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
 +raw_s = state-opaque;
 +raw_s-use_aio = s-use_aio;
 +raw_s-aio_ctx = s-aio_ctx;

...but you are unconditionally assigning into them.  This will introduce
compile failures on other platforms.

 +if ((raw_s-open_flags  ~fcntl_flags) == (s-open_flags  
 ~fcntl_flags)) {
 +/* dup the original fd */
 +/* TODO: use qemu fcntl wrapper */
 +raw_s-fd = fcntl(s-fd, F_DUPFD_CLOEXEC, 0);

F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
your TODO.

 +
 +/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */

s/fctnl/fcntl/

 +static void raw_reopen_commit(BDRVReopenState *state)
 +{
 +BDRVRawReopenState *raw_s = state-opaque;
 +BDRVRawState *s = state-bs-opaque;
 +
 +s-open_flags = raw_s-open_flags;
 +
 +qemu_close(s-fd);
 +s-fd = raw_s-fd;
 +s-use_aio = raw_s-use_aio;
 +s-aio_ctx = raw_s-aio_ctx;

Again, more unconditional use of conditional members.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v3 09/19] block: raw-posix image file reopen

2012-09-18 Thread Jeff Cody 
On 09/18/2012 05:20 PM, Eric Blake wrote:
 On 09/18/2012 12:53 PM, Jeff Cody wrote:
 This is derived from the Supriya Kannery's reopen patches.

 This contains the raw-posix driver changes for the bdrv_reopen_*
 functions.  All changes are staged into a temporary scratch buffer
 during the prepare() stage, and copied over to the live structure
 during commit().  Upon abort(), all changes are abandoned, and the
 live structures are unmodified.

 The _prepare() will create an extra fd - either by means of a dup,
 if possible, or opening a new fd if not (for instance, access
 control changes).  Upon _commit(), the original fd is closed and
 the new fd is used.  Upon _abort(), the duplicate/new fd is closed.

 Signed-off-by: Jeff Cody jc...@redhat.com
 ---
  block/raw-posix.c | 99 
 +++
  1 file changed, 99 insertions(+)

 diff --git a/block/raw-posix.c b/block/raw-posix.c
 index 6bf5480..edd1eca 100644
 --- a/block/raw-posix.c
 +++ b/block/raw-posix.c
 @@ -138,6 +138,15 @@ typedef struct BDRVRawState {
  #endif
  } BDRVRawState;
  
 +typedef struct BDRVRawReopenState {
 +int fd;
 +int open_flags;
 +#ifdef CONFIG_LINUX_AIO
 +int use_aio;
 +void *aio_ctx;
 +#endif
 
 These members are conditional...
 
 +static int raw_reopen_prepare(BDRVReopenState *state,
 +  BlockReopenQueue *queue, Error **errp)
 +{
 +BDRVRawState *s;
 +BDRVRawReopenState *raw_s;
 +int ret = 0;
 +
 +assert(state != NULL);
 +assert(state-bs != NULL);
 +
 +s = state-bs-opaque;
 +
 +state-opaque = g_malloc0(sizeof(BDRVRawReopenState));
 +raw_s = state-opaque;
 +raw_s-use_aio = s-use_aio;
 +raw_s-aio_ctx = s-aio_ctx;
 
 ...but you are unconditionally assigning into them.  This will introduce
 compile failures on other platforms.

Argh.  Thanks.

 
 +if ((raw_s-open_flags  ~fcntl_flags) == (s-open_flags  
 ~fcntl_flags)) {
 +/* dup the original fd */
 +/* TODO: use qemu fcntl wrapper */
 +raw_s-fd = fcntl(s-fd, F_DUPFD_CLOEXEC, 0);
 
 F_DUPFD_CLOEXEC is not defined everywhere yet; you still need to address
 your TODO.
 
 +
 +/* If we cannot use fctnl, or fcntl failed, fall back to qemu_open() */
 
 s/fctnl/fcntl/
 
 +static void raw_reopen_commit(BDRVReopenState *state)
 +{
 +BDRVRawReopenState *raw_s = state-opaque;
 +BDRVRawState *s = state-bs-opaque;
 +
 +s-open_flags = raw_s-open_flags;
 +
 +qemu_close(s-fd);
 +s-fd = raw_s-fd;
 +s-use_aio = raw_s-use_aio;
 +s-aio_ctx = raw_s-aio_ctx;
 
 Again, more unconditional use of conditional members.