block: bdrv_reopen() patches

I believe the bs->keep_read_only flag is supposed to reflect the initial open state of the device. If the device is initially opened R/O, then commit operations, or reopen operations changing to R/W, are prohibited.

Currently, the keep_read_only flag is only accurate for the active layer, and its backing file. Subsequent images end up always having the keep_read_only flag set.

For instance, what happens now:

[ base ] kro = 1, ro = 1

    |
    v
[ snap-1 ]  kro = 1, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

What we want:

[ base ] kro = 0, ro = 1

    |
    v
[ snap-1 ]  kro = 0, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 16 +++++++---------
 block.h |  1 +
 2 files changed, 8 insertions(+), 9 deletions(-)

block.c +1 -3

index 470bdcc..e31b76f 100644
@@ -655,7 +655,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_ALLOW_RDWR);
 
     /*
      * Snapshots should be writable.

@@ -664,8 +664,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
         open_flags |= BDRV_O_RDWR;
     }
 
-    bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
-
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
         ret = drv->bdrv_file_open(bs, filename, open_flags);

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 67 lines

I believe the bs->keep_read_only flag is supposed to reflect the initial open state of the device. If the device is initially opened R/O, then commit operations, or reopen operations changing to R/W, are prohibited.

Currently, the keep_read_only flag is only accurate for the active layer, and its backing file. Subsequent images end up always having the keep_read_only flag set.

For instance, what happens now:

[ base ] kro = 1, ro = 1

    |
    v
[ snap-1 ]  kro = 1, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

What we want:

[ base ] kro = 0, ro = 1

    |
    v
[ snap-1 ]  kro = 0, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 16 +++++++---------
 block.h |  1 +
 2 files changed, 8 insertions(+), 9 deletions(-)

block.c +1 -3

index 470bdcc..e31b76f 100644
@@ -655,7 +655,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_ALLOW_RDWR);
 
     /*
      * Snapshots should be writable.

@@ -664,8 +664,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
         open_flags |= BDRV_O_RDWR;
     }
 
-    bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
-
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
         ret = drv->bdrv_file_open(bs, filename, open_flags);

@@ -804,6 +802,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         goto unlink_and_fail;
     }
 

block.c int bdrv_open(BlockDriverState *bs, const char *filename, int flags, · line 809

Quoted 5 lines

block.c +5 -0

+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
+
+    bs->keep_read_only = !(flags & BDRV_O_ALLOW_RDWR);

Do we still need bs->keep_read_only or is it duplicated in bs->open_flags now? We can convert all users in a follow-up patch.

Quoted 68 lines

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 67 lines

I believe the bs->keep_read_only flag is supposed to reflect the initial open state of the device. If the device is initially opened R/O, then commit operations, or reopen operations changing to R/W, are prohibited.

Currently, the keep_read_only flag is only accurate for the active layer, and its backing file. Subsequent images end up always having the keep_read_only flag set.

For instance, what happens now:

[ base ] kro = 1, ro = 1

    |
    v
[ snap-1 ]  kro = 1, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

What we want:

[ base ] kro = 0, ro = 1

    |
    v
[ snap-1 ]  kro = 0, ro = 1
    |
    v
[ snap-2 ]  kro = 0, ro = 1
    |
    v

[ active ] kro = 0, ro = 0

Signed-off-by Jeff Cody <[email protected]>

---
 block.c | 16 +++++++---------
 block.h |  1 +
 2 files changed, 8 insertions(+), 9 deletions(-)

block.c +1 -3

index 470bdcc..e31b76f 100644
@@ -655,7 +655,7 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
      * Clear flags that are internal to the block layer before opening the
      * image.
      */
-    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
+    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_ALLOW_RDWR);
 
     /*
      * Snapshots should be writable.

@@ -664,8 +664,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename,
         open_flags |= BDRV_O_RDWR;
     }
 
-    bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
-
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
         ret = drv->bdrv_file_open(bs, filename, open_flags);

@@ -804,6 +802,12 @@ int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
         goto unlink_and_fail;
     }
 

Quoted 9 lines

block.c int bdrv_open(BlockDriverState *bs, const char *filename, int flags, · line 809

Quoted 5 lines

block.c +5 -0

+    if (flags & BDRV_O_RDWR) {
+        flags |= BDRV_O_ALLOW_RDWR;
+    }
+
+    bs->keep_read_only = !(flags & BDRV_O_ALLOW_RDWR);

Do we still need bs->keep_read_only or is it duplicated in bs->open_flags now? We can convert all users in a follow-up patch.

I think we can ditch keep_read_only, and just use BDRV_O_ALLOW_RDWR. The only other user of keep_read_only is the original bdrv_commit(), in block.c. And, the natural way to convert that function would be to have it use bdrv_reopen(), to change from r/o->r/w.

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Signed-off-by Jeff Cody <[email protected]>

block.c +40 -0

+/*
+ * Reopen multiple BlockDriverStates atomically & transactionally.
+ *
+ * The queue passed in (bs_queue) must have been built up previous
+ * via bdrv_reopen_queue().
+ *
+ * Reopens all BDS specified in the queue, with the appropriate
+ * flags.  All devices are prepared for reopen, and failure of any
+ * device will cause all device changes to be abandonded, and intermediate
+ * data cleaned up.
+ *
+ * If all devices prepare successfully, then the changes are committed
+ * to all devices.
+ *
+ */
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
+{
+    int ret = -1;
+    BlockReopenQueueEntry *bs_entry;
+    Error *local_err = NULL;
+
+    assert(bs_queue != NULL);
+
+    bdrv_drain_all();
+
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (bdrv_reopen_prepare(bs_entry->state, &local_err)) {
+            error_propagate(errp, local_err);
+            goto cleanup;
+        }
+        bs_entry->prepared = true;
+    }
+
+    /* If we reach this point, we have success and just need to apply the
+     * changes
+     */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        bdrv_reopen_commit(bs_entry->state);
+    }
+

block.c +39 -0

+    g_free(bs_queue);
+    return ret;
+}
+
+
+/* Reopen a single BlockDriverState with the specified flags. */
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
+{
+    int ret = -1;
+    Error *local_err = NULL;
+    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+
+    ret = bdrv_reopen_multiple(queue, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+    }
+    return ret;
+}
+
+
+/*
+ * Prepares a BlockDriverState for reopen. All changes are staged in the
+ * 'reopen_state' field of the BlockDriverState, which must be NULL when
+ * entering (all previous reopens must have completed for the BDS).
+ *
+ * bs is the BlockDriverState to reopen
+ * flags are the new open flags
+ *
+ * Returns 0 on success, non-zero on error.  On error errp will be set
+ * as well.
+ *
+ * On failure, bdrv_reopen_abort() will be called to clean up any data.
+ * It is the responsibility of the caller to then call the abort() or
+ * commit() for any other BDS that have been left in a prepare() state
+ *
+ */
+int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp)
+{
+    int ret = -1;

block.c +16 -0

+
+    if (drv->bdrv_reopen_prepare) {
+        ret = drv->bdrv_reopen_prepare(reopen_state, &local_err);
+        if (ret) {
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+            } else {
+                error_set(errp, QERR_OPEN_FILE_FAILED,
+                          reopen_state->bs->filename);
+            }
+            goto error;
+        }
+    } else {
+        /* It is currently mandatory to have a bdrv_reopen_prepare()
+         * handler for each supported drv. */
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,

block.c +16 -0

+    return ret;
+}
+
+/*
+ * Takes the staged changes for the reopen from bdrv_reopen_prepare(), and
+ * makes them final by swapping the staging BlockDriverState contents into
+ * the active BlockDriverState contents.
+ */
+void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+

block.c +20 -0

+}
+
+/*
+ * Abort the reopen, and delete and free the staged changes in
+ * reopen_state
+ */
+void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+
+    if (drv->bdrv_reopen_abort) {
+        drv->bdrv_reopen_abort(reopen_state);
+    }
+}
+
+
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);

block.h +0 -0

index 4d919c2..db812b1 100644

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 59 lines

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Signed-off-by Jeff Cody <[email protected]>

block.c +44 -0

+/*
+ * Reopen multiple BlockDriverStates atomically & transactionally.
+ *
+ * The queue passed in (bs_queue) must have been built up previous
+ * via bdrv_reopen_queue().
+ *
+ * Reopens all BDS specified in the queue, with the appropriate
+ * flags.  All devices are prepared for reopen, and failure of any
+ * device will cause all device changes to be abandonded, and intermediate
+ * data cleaned up.
+ *
+ * If all devices prepare successfully, then the changes are committed
+ * to all devices.
+ *
+ */
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
+{
+    int ret = -1;
+    BlockReopenQueueEntry *bs_entry;
+    Error *local_err = NULL;
+
+    assert(bs_queue != NULL);
+
+    bdrv_drain_all();
+
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (bdrv_reopen_prepare(bs_entry->state, &local_err)) {
+            error_propagate(errp, local_err);
+            goto cleanup;
+        }
+        bs_entry->prepared = true;
+    }
+
+    /* If we reach this point, we have success and just need to apply the
+     * changes
+     */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        bdrv_reopen_commit(bs_entry->state);
+    }
+
+    ret = 0;
+
+cleanup:
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {

block.c unlink_and_fail: · line 952

Quoted 6 lines

block.c +6 -0

+        if (ret && bs_entry->prepared) {
+            bdrv_reopen_abort(bs_entry->state);
+        }
+        g_free(bs_entry->state);
+        g_free(bs_entry);
+    }

Without QSIMPLEQ_FOREACH_SAFE, isn't this a use after free?

Quoted 39 lines

block.c +39 -0

+    g_free(bs_queue);
+    return ret;
+}
+
+
+/* Reopen a single BlockDriverState with the specified flags. */
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
+{
+    int ret = -1;
+    Error *local_err = NULL;
+    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+
+    ret = bdrv_reopen_multiple(queue, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+    }
+    return ret;
+}
+
+
+/*
+ * Prepares a BlockDriverState for reopen. All changes are staged in the
+ * 'reopen_state' field of the BlockDriverState, which must be NULL when
+ * entering (all previous reopens must have completed for the BDS).
+ *
+ * bs is the BlockDriverState to reopen
+ * flags are the new open flags
+ *
+ * Returns 0 on success, non-zero on error.  On error errp will be set
+ * as well.
+ *
+ * On failure, bdrv_reopen_abort() will be called to clean up any data.
+ * It is the responsibility of the caller to then call the abort() or
+ * commit() for any other BDS that have been left in a prepare() state
+ *
+ */
+int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp)
+{
+    int ret = -1;

block.c unlink_and_fail: · line 1001

Quoted 10 lines

block.c +10 -0

+    Error *local_err = NULL;
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    assert(reopen_state->bs->drv != NULL);
+    drv = reopen_state->bs->drv;
+
+    /* if we are to stay read-only, do not allow permission change
+     * to r/w */
+    if (reopen_state->bs->keep_read_only &&

Just for completeness, we decided to use the flag here instead of keep_read_only.

block.c unlink_and_fail: · line 1013

Quoted 12 lines

block.c +12 -0

+        reopen_state->flags & BDRV_O_RDWR) {
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY,
+                  reopen_state->bs->device_name);
+        goto error;
+    }
+
+
+    ret = bdrv_flush(reopen_state->bs);
+    if (ret) {
+        error_set(errp, QERR_IO_ERROR);
+        goto error;
+    }

This throws the error code away. Bad. We should probably change QERR_IO_ERROR so that you can include strerror(-ret).

Quoted 16 lines

block.c +16 -0

+
+    if (drv->bdrv_reopen_prepare) {
+        ret = drv->bdrv_reopen_prepare(reopen_state, &local_err);
+        if (ret) {
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+            } else {
+                error_set(errp, QERR_OPEN_FILE_FAILED,
+                          reopen_state->bs->filename);
+            }
+            goto error;
+        }
+    } else {
+        /* It is currently mandatory to have a bdrv_reopen_prepare()
+         * handler for each supported drv. */
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,

block.c unlink_and_fail: · line 1039

Quoted 10 lines

block.c +10 -0

+                  drv->format_name, reopen_state->bs->device_name,
+                 "reopening of file");
+        ret = -1;
+        goto error;
+    }
+
+    return 0;
+
+error:
+    bdrv_reopen_abort(reopen_state);

This is unexpected for me. Shouldn't .bdrv_reopen_prepare() clean up before returning an error, like any other function does? (Which could actually be a call to bdrv_reopen_abort() where it makes sense.)

If you use .bdrv_reopen_abort() for it, block drivers must take care to write this function in way that doesn't assume that .bdrv_reopen_prepare() has completed. Sounds rather nasty to me.

Quoted 21 lines

block.c +21 -0

+    return ret;
+}
+
+/*
+ * Takes the staged changes for the reopen from bdrv_reopen_prepare(), and
+ * makes them final by swapping the staging BlockDriverState contents into
+ * the active BlockDriverState contents.
+ */
+void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+
+    /* If there are any driver level actions to take */
+    if (drv->bdrv_reopen_commit) {
+        drv->bdrv_reopen_commit(reopen_state);
+    }
+

block.c unlink_and_fail: · line 1065

Quoted 5 lines

block.c +5 -0

+    /* set BDS specific flags now */
+    reopen_state->bs->open_flags         = reopen_state->flags;
+    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
+                                              BDRV_O_CACHE_WB);
+    reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);

Hm, I wonder if these three lines can somehow be shared with the normal bdrv_open so that they stay in sync.

Quoted 29 lines

block.c +20 -0

+}
+
+/*
+ * Abort the reopen, and delete and free the staged changes in
+ * reopen_state
+ */
+void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+
+    if (drv->bdrv_reopen_abort) {
+        drv->bdrv_reopen_abort(reopen_state);
+    }
+}
+
+
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);

block.h +0 -0

index 4d919c2..db812b1 100644

block.h typedef enum { · line 102

Quoted 6 lines

block.h +3 -0

@@ -97,6 +97,14 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockQMPEventAction;
 
+typedef struct BlockReopenQueueEntry {
+     bool prepared;
+     BDRVReopenState *state;

As discussed on IRC, this can be directly embedded instead of using a pointer.

block.h typedef enum { · line 104

Quoted 2 lines

block.h +2 -0

+     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+} BlockReopenQueueEntry;

Kevin

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Signed-off-by Jeff Cody <[email protected]>

---
 block.c       | 226 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block.h       |  15 ++++
 block_int.h   |  13 ++++
 qemu-common.h |   1 +
 4 files changed, 255 insertions(+)

block.c +226 -0

index e31b76f..9470319 100644
@@ -857,6 +857,232 @@ unlink_and_fail:
     return ret;
 }
 
+/*
+ * Adds a BlockDriverState to a simple queue for an atomic, transactional
+ * reopen of multiple devices.
+ *
+ * bs_queue can either be an existing BlockReopenQueue that has had QSIMPLE_INIT
+ * already performed, or alternatively may be NULL a new BlockReopenQueue will
+ * be created and initialized. This newly created BlockReopenQueue should be
+ * passed back in for subsequent calls that are intended to be of the same
+ * atomic 'set'.
+ *
+ * bs is the BlockDriverState to add to the reopen queue.
+ *
+ * flags contains the open flags for the associated bs
+ *
+ * returns a pointer to bs_queue, which is either the newly allocated
+ * bs_queue, or the existing bs_queue being used.
+ *
+ */
+BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
+                                    BlockDriverState *bs, int flags)
+{
+    assert(bs != NULL);
+
+    BlockReopenQueueEntry *bs_entry;
+    if (bs_queue == NULL) {
+        bs_queue = g_new0(BlockReopenQueue, 1);
+        QSIMPLEQ_INIT(bs_queue);
+    }
+
+    if (bs->file) {
+        bdrv_reopen_queue(bs_queue, bs->file, flags);
+    }
+
+    bs_entry = g_new0(BlockReopenQueueEntry, 1);
+    QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+
+    bs_entry->state = g_new0(BDRVReopenState, 1);
+    bs_entry->state->bs = bs;
+    bs_entry->state->flags = flags;
+
+    return bs_queue;
+}
+
+/*
+ * Reopen multiple BlockDriverStates atomically & transactionally.
+ *
+ * The queue passed in (bs_queue) must have been built up previous
+ * via bdrv_reopen_queue().
+ *
+ * Reopens all BDS specified in the queue, with the appropriate
+ * flags.  All devices are prepared for reopen, and failure of any
+ * device will cause all device changes to be abandonded, and intermediate
+ * data cleaned up.
+ *
+ * If all devices prepare successfully, then the changes are committed
+ * to all devices.
+ *
+ */
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
+{
+    int ret = -1;
+    BlockReopenQueueEntry *bs_entry;
+    Error *local_err = NULL;
+
+    assert(bs_queue != NULL);
+
+    bdrv_drain_all();
+
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (bdrv_reopen_prepare(bs_entry->state, &local_err)) {
+            error_propagate(errp, local_err);
+            goto cleanup;
+        }
+        bs_entry->prepared = true;
+    }
+
+    /* If we reach this point, we have success and just need to apply the
+     * changes
+     */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        bdrv_reopen_commit(bs_entry->state);
+    }
+
+    ret = 0;
+
+cleanup:
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        if (ret && bs_entry->prepared) {
+            bdrv_reopen_abort(bs_entry->state);
+        }
+        g_free(bs_entry->state);
+        g_free(bs_entry);
+    }
+    g_free(bs_queue);
+    return ret;
+}
+
+
+/* Reopen a single BlockDriverState with the specified flags. */
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
+{
+    int ret = -1;
+    Error *local_err = NULL;
+    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+
+    ret = bdrv_reopen_multiple(queue, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+    }
+    return ret;
+}
+
+
+/*
+ * Prepares a BlockDriverState for reopen. All changes are staged in the
+ * 'reopen_state' field of the BlockDriverState, which must be NULL when
+ * entering (all previous reopens must have completed for the BDS).
+ *
+ * bs is the BlockDriverState to reopen
+ * flags are the new open flags
+ *
+ * Returns 0 on success, non-zero on error.  On error errp will be set
+ * as well.
+ *
+ * On failure, bdrv_reopen_abort() will be called to clean up any data.
+ * It is the responsibility of the caller to then call the abort() or
+ * commit() for any other BDS that have been left in a prepare() state
+ *
+ */
+int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp)
+{
+    int ret = -1;
+    Error *local_err = NULL;
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    assert(reopen_state->bs->drv != NULL);
+    drv = reopen_state->bs->drv;
+
+    /* if we are to stay read-only, do not allow permission change
+     * to r/w */
+    if (reopen_state->bs->keep_read_only &&
+        reopen_state->flags & BDRV_O_RDWR) {
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY,
+                  reopen_state->bs->device_name);
+        goto error;
+    }
+
+
+    ret = bdrv_flush(reopen_state->bs);
+    if (ret) {
+        error_set(errp, QERR_IO_ERROR);
+        goto error;
+    }
+
+    if (drv->bdrv_reopen_prepare) {
+        ret = drv->bdrv_reopen_prepare(reopen_state, &local_err);
+        if (ret) {
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+            } else {
+                error_set(errp, QERR_OPEN_FILE_FAILED,
+                          reopen_state->bs->filename);
+            }
+            goto error;
+        }
+    } else {
+        /* It is currently mandatory to have a bdrv_reopen_prepare()
+         * handler for each supported drv. */
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  drv->format_name, reopen_state->bs->device_name,
+                 "reopening of file");
+        ret = -1;
+        goto error;
+    }
+
+    return 0;
+
+error:
+    bdrv_reopen_abort(reopen_state);
+    return ret;
+}
+
+/*
+ * Takes the staged changes for the reopen from bdrv_reopen_prepare(), and
+ * makes them final by swapping the staging BlockDriverState contents into
+ * the active BlockDriverState contents.
+ */
+void bdrv_reopen_commit(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+
+    /* If there are any driver level actions to take */
+    if (drv->bdrv_reopen_commit) {
+        drv->bdrv_reopen_commit(reopen_state);
+    }
+
+    /* set BDS specific flags now */
+    reopen_state->bs->open_flags         = reopen_state->flags;
+    reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
+                                              BDRV_O_CACHE_WB);
+    reopen_state->bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+}
+
+/*
+ * Abort the reopen, and delete and free the staged changes in
+ * reopen_state
+ */
+void bdrv_reopen_abort(BDRVReopenState *reopen_state)
+{
+    BlockDriver *drv;
+
+    assert(reopen_state != NULL);
+    drv = reopen_state->bs->drv;
+    assert(drv != NULL);
+
+    if (drv->bdrv_reopen_abort) {
+        drv->bdrv_reopen_abort(reopen_state);
+    }
+}
+
+
 void bdrv_close(BlockDriverState *bs)
 {
     bdrv_flush(bs);

block.h +15 -0

index 4d919c2..db812b1 100644
@@ -97,6 +97,14 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockQMPEventAction;
 
+typedef struct BlockReopenQueueEntry {
+     bool prepared;
+     BDRVReopenState *state;
+     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+} BlockReopenQueueEntry;
+
+typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
+
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
 void bdrv_iostatus_disable(BlockDriverState *bs);

@@ -131,6 +139,13 @@ int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
               BlockDriver *drv);
+BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
+                                    BlockDriverState *bs, int flags);
+int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
+int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
+int bdrv_reopen_prepare(BDRVReopenState *reopen_state, Error **errp);
+void bdrv_reopen_commit(BDRVReopenState *reopen_state);
+void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 void bdrv_close(BlockDriverState *bs);
 int bdrv_attach_dev(BlockDriverState *bs, void *dev);
 void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);

block_int.h +13 -0

index 4452f6f..7a4e226 100644
@@ -139,6 +139,12 @@ struct BlockDriver {
     int instance_size;
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
     int (*bdrv_probe_device)(const char *filename);
+
+    /* For handling image reopen for split or non-split files */
+    int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state, Error **errp);
+    void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
+    void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
+
     int (*bdrv_open)(BlockDriverState *bs, int flags);
     int (*bdrv_file_open)(BlockDriverState *bs, const char *filename, int flags);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,

@@ -336,6 +342,13 @@ struct BlockDriverState {
 
     /* long-running background operation */
     BlockJob *job;
+
+};
+
+struct BDRVReopenState {
+    BlockDriverState *bs;
+    int flags;
+    void *opaque;
 };
 
 int get_tmp_filename(char *filename, int size);

qemu-common.h +1 -0

index e5c2bcd..6a6181c 100644
@@ -245,6 +245,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;

Quoted 6 lines

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Quoted 7 lines

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Open question (my assumption is yes):

Is it safe to assume that reopen() should always enable BDRV_O_CACHE_WB (without affecting enable_write_cache), so as to not undo what was done by Paolo's commit e1e9b0ac?

Am 11.09.2012 16:57, schrieb Jeff Cody:

Quoted 17 lines · Jeff Cody

Quoted 6 lines

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Quoted 7 lines

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Open question (my assumption is yes):

Quoted 3 lines

Is it safe to assume that reopen() should always enable BDRV_O_CACHE_WB (without affecting enable_write_cache), so as to not undo what was done by Paolo's commit e1e9b0ac?

I think it makes sense to behave the same as bdrv_open_common(), so I guess yes. But now I'm wondering if we also need other code from there, like filtering out BDRV_O_SNAPSHOT/NO_BACKING etc.

Quoted 18 lines

Am 11.09.2012 16:57, schrieb Jeff Cody:

Quoted 17 lines · Jeff Cody

Quoted 6 lines

This is based heavily on Supriya Kannery's bdrv_reopen() patch series.

This provides a transactional method to reopen multiple images files safely.

Quoted 7 lines

Image files are queue for reopen via bdrv_reopen_queue(), and the reopen occurs when bdrv_reopen_multiple() is called. Changes are staged in bdrv_reopen_prepare() and in the equivalent driver level functions. If any of the staged images fails a prepare, then all of the images left untouched, and the staged changes for each image abandoned.

Open question (my assumption is yes):

Quoted 8 lines

Quoted 3 lines

Is it safe to assume that reopen() should always enable BDRV_O_CACHE_WB (without affecting enable_write_cache), so as to not undo what was done by Paolo's commit e1e9b0ac?

I think it makes sense to behave the same as bdrv_open_common(), so I guess yes. But now I'm wondering if we also need other code from there, like filtering out BDRV_O_SNAPSHOT/NO_BACKING etc.

I was wondering the same thing w/regards to BDRV_O_SNAPSHOT/NO_BACKING, but I fell on the side of 'no'. Mainly because the raw drivers (raw-posix, raw-win32) actively parse the passed flags to determine the actual open flags, and so spurious flags such as those are ignored. However, BDRV_O_CACHE_WB is used in their flag parsing logic, so I think it needs to be preserved.

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.

Quoted 13 lines · 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.

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 347

Quoted 4 lines

block/raw-posix.c +4 -0

+    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);

I assume this TODO has to be fixed to allow compilation on systems that lack F_DUPFD_CLOEXEC.

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 354

Quoted 8 lines

block/raw-posix.c +7 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Is raw_s->open_flags every going to contain O_CREAT, or is the 0644 mode argument spurious?

Quoted 20 lines · Jeff Cody

Quoted 18 lines

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.

block/raw-posix.c +4 -0

+    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);

Quoted 2 lines

I assume this TODO has to be fixed to allow compilation on systems that lack F_DUPFD_CLOEXEC.

Yes, either that or add the logic here.

Quoted 8 lines

Quoted 7 lines

block/raw-posix.c +7 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 2 lines

Is raw_s->open_flags every going to contain O_CREAT, or is the 0644 mode argument spurious?

Thanks, you are right, it is spurious. The raw_s->open_flags are explicitly set via raw_parse_flags(), so we know it will never contain O_CREAT.

Am 31.08.2012 16:42, schrieb Jeff Cody:

Quoted 21 lines · Eric Blake

Quoted 20 lines · Jeff Cody

Quoted 18 lines

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.

block/raw-posix.c +4 -0

+    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);

Quoted 4 lines

Quoted 2 lines

I assume this TODO has to be fixed to allow compilation on systems that lack F_DUPFD_CLOEXEC.

Yes, either that or add the logic here.

Would qemu_dup_flags() from osdep.c be the right thing here? It was introduces with Corey's fd passing series.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

block/raw-posix.c +7 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 6 lines

Quoted 2 lines

Is raw_s->open_flags every going to contain O_CREAT, or is the 0644 mode argument spurious?

Thanks, you are right, it is spurious. The raw_s->open_flags are explicitly set via raw_parse_flags(), so we know it will never contain O_CREAT.

We can probably assert it.

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 <[email protected]>

---
 block/raw-posix.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 139 insertions(+), 14 deletions(-)

block/raw-posix.c +20 -0

index 6be20b1..48086d7 100644
@@ -140,6 +140,15 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    BDRVReopenState reopen_state;
+    int fd;
+    int open_flags;
+    uint8_t *aligned_buf;
+    unsigned aligned_buf_size;
+    BDRVRawState *stash_s;
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 

@@ -185,6 +194,28 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
+static void raw_parse_flags(int bdrv_flags, int *open_flags)
+{
+    assert(open_flags != NULL);
+
+    *open_flags |= O_BINARY;
+    *open_flags &= ~O_ACCMODE;
+    if (bdrv_flags & BDRV_O_RDWR) {
+        *open_flags |= O_RDWR;
+    } else {
+        *open_flags |= O_RDONLY;
+    }

block/raw-posix.c +26 -14

+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
                            int bdrv_flags, int open_flags)
 {

@@ -196,20 +227,8 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
         return ret;
     }
 
-    s->open_flags = open_flags | O_BINARY;
-    s->open_flags &= ~O_ACCMODE;
-    if (bdrv_flags & BDRV_O_RDWR) {
-        s->open_flags |= O_RDWR;
-    } else {
-        s->open_flags |= O_RDONLY;
-    }
-
-    /* Use O_DSYNC for write-through caching, no flags for write-back caching,
-     * and O_DIRECT for no caching. */
-    if ((bdrv_flags & BDRV_O_NOCACHE))
-        s->open_flags |= O_DIRECT;
-    if (!(bdrv_flags & BDRV_O_CACHE_WB))
-        s->open_flags |= O_DSYNC;
+    s->open_flags = open_flags;
+    raw_parse_flags(bdrv_flags, &s->open_flags);
 
     s->fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);

@@ -283,6 +302,109 @@ 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, 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_parse_flags(state->flags, &raw_s->open_flags);
+
+    /*
+     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
+     * aligned_buf
+     */
+    if ((state->flags & BDRV_O_NOCACHE)) {
+        /*
+         * Allocate a buffer for read/modify/write cycles.  Choose the size

block/raw-posix.c +10 -0

+    }
+
+    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);

block/raw-posix.c +17 -0

+    }
+error:
+    return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+
+    if (raw_s->aligned_buf != NULL) {
+        if (s->aligned_buf) {
+            qemu_vfree(s->aligned_buf);
+        }
+        s->aligned_buf      = raw_s->aligned_buf;
+        s->aligned_buf_size = raw_s->aligned_buf_size;

Am 30.08.2012 20:47, schrieb Jeff Cody:

Quoted 57 lines

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 <[email protected]>

---
 block/raw-posix.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 139 insertions(+), 14 deletions(-)

block/raw-posix.c +24 -0

index 6be20b1..48086d7 100644
@@ -140,6 +140,15 @@ typedef struct BDRVRawState {
 #endif
 } BDRVRawState;
 
+typedef struct BDRVRawReopenState {
+    BDRVReopenState reopen_state;
+    int fd;
+    int open_flags;
+    uint8_t *aligned_buf;
+    unsigned aligned_buf_size;
+    BDRVRawState *stash_s;
+} BDRVRawReopenState;
+
 static int fd_open(BlockDriverState *bs);
 static int64_t raw_getlength(BlockDriverState *bs);
 

@@ -185,6 +194,28 @@ static int raw_normalize_devicepath(const char **filename)
 }
 #endif
 
+static void raw_parse_flags(int bdrv_flags, int *open_flags)
+{
+    assert(open_flags != NULL);
+
+    *open_flags |= O_BINARY;
+    *open_flags &= ~O_ACCMODE;
+    if (bdrv_flags & BDRV_O_RDWR) {
+        *open_flags |= O_RDWR;
+    } else {
+        *open_flags |= O_RDONLY;
+    }
+
+    /* Use O_DSYNC for write-through caching, no flags for write-back caching,
+     * and O_DIRECT for no caching. */
+    if ((bdrv_flags & BDRV_O_NOCACHE)) {

block/raw-posix.c static int raw_normalize_devicepath(const char **filename) · line 217

Quoted 6 lines

block/raw-posix.c +6 -0

+        *open_flags |= O_DIRECT;
+    }
+    if (!(bdrv_flags & BDRV_O_CACHE_WB)) {
+        *open_flags |= O_DSYNC;
+    }
+}

The code motion would ideally be a separate patch.

Quoted 59 lines

block/raw-posix.c +31 -14

+
 static int raw_open_common(BlockDriverState *bs, const char *filename,
                            int bdrv_flags, int open_flags)
 {

@@ -196,20 +227,8 @@ static int raw_open_common(BlockDriverState *bs, const char *filename,
         return ret;
     }
 
-    s->open_flags = open_flags | O_BINARY;
-    s->open_flags &= ~O_ACCMODE;
-    if (bdrv_flags & BDRV_O_RDWR) {
-        s->open_flags |= O_RDWR;
-    } else {
-        s->open_flags |= O_RDONLY;
-    }
-
-    /* Use O_DSYNC for write-through caching, no flags for write-back caching,
-     * and O_DIRECT for no caching. */
-    if ((bdrv_flags & BDRV_O_NOCACHE))
-        s->open_flags |= O_DIRECT;
-    if (!(bdrv_flags & BDRV_O_CACHE_WB))
-        s->open_flags |= O_DSYNC;
+    s->open_flags = open_flags;
+    raw_parse_flags(bdrv_flags, &s->open_flags);
 
     s->fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);

@@ -283,6 +302,109 @@ 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, 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_parse_flags(state->flags, &raw_s->open_flags);
+
+    /*
+     * If we didn't have BDRV_O_NOCACHE set before, we may not have allocated
+     * aligned_buf
+     */
+    if ((state->flags & BDRV_O_NOCACHE)) {
+        /*
+         * Allocate a buffer for read/modify/write cycles.  Choose the size
+         * pessimistically as we don't know the block size yet.
+         */
+        raw_s->aligned_buf_size = 32 * MAX_BLOCKSIZE;
+        raw_s->aligned_buf = qemu_memalign(MAX_BLOCKSIZE,
+                                           raw_s->aligned_buf_size);

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 337

Quoted 5 lines

block/raw-posix.c +5 -0

+
+        if (raw_s->aligned_buf == NULL) {
+            ret = -1;
+            goto error;
+        }

Even though it's pretty small, I think I would factor this out into a small static helper to make sure it's kept in sync with raw_open_common().

Quoted 17 lines

block/raw-posix.c +17 -0

+    }
+
+    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 == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 351

Quoted 3 lines

block/raw-posix.c +3 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a bit different. In particular, I'm hoping that we don't get a second fcntl_flags enumeration here, but can just fall back to qemu_open() whenever qemu_dup_flags() fails.

If we do need to keep fcntl_flags here, we'll probably want to add O_DIRECT to it.

Quoted 23 lines

block/raw-posix.c +23 -0

+    }
+error:
+    return ret;
+}
+
+
+static void raw_reopen_commit(BDRVReopenState *state)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+
+    if (raw_s->aligned_buf != NULL) {
+        if (s->aligned_buf) {
+            qemu_vfree(s->aligned_buf);
+        }
+        s->aligned_buf      = raw_s->aligned_buf;
+        s->aligned_buf_size = raw_s->aligned_buf_size;
+    }
+
+    s->open_flags = raw_s->open_flags;
+
+    close(s->fd);
+    s->fd = raw_s->fd;

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 384

Quoted 4 lines

block/raw-posix.c +4 -0

+
+    g_free(state->opaque);
+    state->opaque = NULL;
+}

I think s->use_aio must be changed as well, it depends on BDRV_O_NOCACHE. Maybe we even need to do it in prepare, laio_init() may need to be called.

Kevin

Quoted 17 lines

Quoted 17 lines

block/raw-posix.c +17 -0

+    }
+
+    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 == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 8 lines

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 351

Quoted 3 lines

block/raw-posix.c +3 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a bit different. In particular, I'm hoping that we don't get a second fcntl_flags enumeration here, but can just fall back to qemu_open() whenever qemu_dup_flags() fails.

That will require modification to qemu_dup_flags()... I believe qemu_dup_flags() silently filters out fcntl incompatible flags.

Maybe it would be best to create a small helper function in osdep.c, that fetches the fcntl_flags. Then qemu_dup_flags() and this function would use the same helper to fetch fcntl_flags. The results of that would determine if we call qemu_dup_flags() or qemu_open().

Although, I do think it makes sense to always try qemu_open() if qemu_dup_flags() fails for some reason.

Am 05.09.2012 18:43, schrieb Jeff Cody:

Quoted 36 lines

Quoted 17 lines

Quoted 17 lines

block/raw-posix.c +17 -0

+    }
+
+    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 == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 8 lines

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 351

Quoted 3 lines

block/raw-posix.c +3 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a bit different. In particular, I'm hoping that we don't get a second fcntl_flags enumeration here, but can just fall back to qemu_open() whenever qemu_dup_flags() fails.

That will require modification to qemu_dup_flags()... I believe qemu_dup_flags() silently filters out fcntl incompatible flags.

Maybe it would be best to create a small helper function in osdep.c, that fetches the fcntl_flags. Then qemu_dup_flags() and this function would use the same helper to fetch fcntl_flags. The results of that would determine if we call qemu_dup_flags() or qemu_open().

Although, I do think it makes sense to always try qemu_open() if qemu_dup_flags() fails for some reason.

If we can modify qemu_dup_flags() to fail if it can't provide the right set of flags, then I think we should do it - and I think we can. Even for the existing cases with fd passing it shouldn't break anything, but only add an additional safety check.

And if touching the function motivates Corey to write some fd passing test cases so that you can't break it, even better. ;-)

Quoted 35 lines

Am 05.09.2012 18:43, schrieb Jeff Cody:

Quoted 34 lines

Quoted 17 lines

Quoted 17 lines

block/raw-posix.c +17 -0

+    }
+
+    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 == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 8 lines

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 351

Quoted 3 lines

block/raw-posix.c +3 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a bit different. In particular, I'm hoping that we don't get a second fcntl_flags enumeration here, but can just fall back to qemu_open() whenever qemu_dup_flags() fails.

That will require modification to qemu_dup_flags()... I believe qemu_dup_flags() silently filters out fcntl incompatible flags.

Maybe it would be best to create a small helper function in osdep.c, that fetches the fcntl_flags. Then qemu_dup_flags() and this function would use the same helper to fetch fcntl_flags. The results of that would determine if we call qemu_dup_flags() or qemu_open().

Quoted 2 lines

Quoted 2 lines

Although, I do think it makes sense to always try qemu_open() if qemu_dup_flags() fails for some reason.

I'm curious why you can't always call qemu_open().

Some things to consider so that fd passing doesn't break when a reopen occurs. Mainly all the concerns revolve around how fd passing keeps track of references to fd sets (note: adding and removing fd set references is all done in qemu_open and qemu_close).

* When reopening, qemu_open needs to be called before qemu_close. This will prevent the reference list for an fdset from becoming empty. If qemu_close is called before qemu_open, the reference list can become empty, and the fdset could be cleaned up before the qemu_open. Then qemu_open would fail.

* qemu_open/qemu_close need to be used rather than open/close so that the references for fd passing are properly accounted for.

* I don't think you want to call qemu_dup_flags directly since it doesn't update the reference list for fd passing. Only qemu_open and qemu_close update the reference list.

Quoted 7 lines

If we can modify qemu_dup_flags() to fail if it can't provide the right set of flags, then I think we should do it - and I think we can. Even for the existing cases with fd passing it shouldn't break anything, but only add an additional safety check.

And if touching the function motivates Corey to write some fd passing test cases so that you can't break it, even better. ;-)

:) Sorry, I do plan to do this soon. I've just been side-tracked with some other things.

Am 06.09.2012 17:34, schrieb Corey Bryant:

Quoted 38 lines · Kevin Wolf

Quoted 35 lines

Am 05.09.2012 18:43, schrieb Jeff Cody:

Quoted 34 lines

Quoted 17 lines

Quoted 17 lines

block/raw-posix.c +17 -0

+    }
+
+    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 == -1) {
+            ret = -1;
+            goto error;
+        }
+        ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
+    } else {
+        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags, 0644);

Quoted 8 lines

block/raw-posix.c static int raw_open(BlockDriverState *bs, const char *filename, int flags) · line 351

Quoted 3 lines

block/raw-posix.c +3 -0

+        if (raw_s->fd == -1) {
+            ret = -1;
+        }

Ignoring this part for now, with qemu_dup_flags() it's going to look a bit different. In particular, I'm hoping that we don't get a second fcntl_flags enumeration here, but can just fall back to qemu_open() whenever qemu_dup_flags() fails.

That will require modification to qemu_dup_flags()... I believe qemu_dup_flags() silently filters out fcntl incompatible flags.

Maybe it would be best to create a small helper function in osdep.c, that fetches the fcntl_flags. Then qemu_dup_flags() and this function would use the same helper to fetch fcntl_flags. The results of that would determine if we call qemu_dup_flags() or qemu_open().

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

Although, I do think it makes sense to always try qemu_open() if qemu_dup_flags() fails for some reason.

I'm curious why you can't always call qemu_open().

I believe the original reason was that qemu_open() is more likely to fail, for example if the image file has been renamed/moved/deleted since the first open. You could still use fcntl() on an existing file descriptor, but reopening would fail.

Quoted 10 lines

Some things to consider so that fd passing doesn't break when a reopen occurs. Mainly all the concerns revolve around how fd passing keeps track of references to fd sets (note: adding and removing fd set references is all done in qemu_open and qemu_close).

* When reopening, qemu_open needs to be called before qemu_close. This will prevent the reference list for an fdset from becoming empty. If qemu_close is called before qemu_open, the reference list can become empty, and the fdset could be cleaned up before the qemu_open. Then qemu_open would fail.

Will automatically be right when we properly implement transactional semantics.

Quoted 2 lines

* qemu_open/qemu_close need to be used rather than open/close so that the references for fd passing are properly accounted for.

Congratulations, you've just discovered a bug in Jeff's patches. It was a good idea to CC you. ;-)

Quoted 3 lines

* I don't think you want to call qemu_dup_flags directly since it doesn't update the reference list for fd passing. Only qemu_open and qemu_close update the reference list.

That's a good point, too. So probably a small wrapper that just updates the reference list in addition?

Quoted 10 lines

Quoted 7 lines

If we can modify qemu_dup_flags() to fail if it can't provide the right set of flags, then I think we should do it - and I think we can. Even for the existing cases with fd passing it shouldn't break anything, but only add an additional safety check.

And if touching the function motivates Corey to write some fd passing test cases so that you can't break it, even better. ;-)

:) Sorry, I do plan to do this soon. I've just been side-tracked with some other things.

No problem, it was just such a great opportunity to remind you. ;-)

Kevin