Modify block jobs to use node-names

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

block.c +15 -1

index 43abe96..da32bb0 100644
@@ -843,12 +843,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+#define GEN_NODE_NAME_PREFIX    "__qemu##"
+#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
+    static uint32_t counter; /* simple counter to guarantee uniqueness */
+
+    /* if node_name is NULL, auto-generate a node name */
     if (!node_name) {
-        return;
+        int len;
+        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
+                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
+        len = strlen(gen_node_name);
+        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
+            gen_node_name[len++] = g_random_int_range('A', 'Z');
+        }
+        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
+        node_name = gen_node_name;
     }
 
     /* empty string node name is invalid */

Signature · 1 line

1.9.3

Quoted 13 lines · Jeff Cody

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

Quoted 7 lines

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

Jeff can't we simply enforce the namespace separation with a check on the QDict option content ?
This way we could be sure that the user can't input a node-name starting with __qemu.

Quoted 47 lines

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

block.c +15 -1

index 43abe96..da32bb0 100644
@@ -843,12 +843,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+#define GEN_NODE_NAME_PREFIX    "__qemu##"
+#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
+    static uint32_t counter; /* simple counter to guarantee uniqueness */
+
+    /* if node_name is NULL, auto-generate a node name */
     if (!node_name) {
-        return;
+        int len;
+        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
+                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
+        len = strlen(gen_node_name);
+        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
+            gen_node_name[len++] = g_random_int_range('A', 'Z');
+        }
+        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
+        node_name = gen_node_name;
     }
 
     /* empty string node name is invalid */

Signature · 1 line

1.9.3

Quoted 22 lines · Benoît Canet

Quoted 13 lines · Jeff Cody

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

Quoted 7 lines

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

Quoted 5 lines

Jeff can't we simply enforce the namespace separation with a check on the QDict option content ?
This way we could be sure that the user can't input a node-name starting with __qemu.

That still would not stop a user from trying to 'predict' or assuming what a node name would be ("oh, it is the first drive, it is probably __qemu##0000", etc...). Having the combination of the incrementing counter and the random string generation guarantees 2 things: it will always be unique in a qemu session, and it is not predictable by the user. The "__qemu##" just helps to visually identify it as a qemu generated.

Although if you are strictly concerned about namespace confusion, we could enforce the namespace as you suggest, so a user could not create a node-name that would look like a qemu-generated node-name. Even in that case, I would still want to keep the sequential number + random string.

Quoted 48 lines

Quoted 48 lines

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

block.c +15 -1

index 43abe96..da32bb0 100644
@@ -843,12 +843,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
     return open_flags;
 }
 
+#define GEN_NODE_NAME_PREFIX    "__qemu##"
+#define GEN_NODE_NAME_MAX_LEN   (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
 static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char gen_node_name[GEN_NODE_NAME_MAX_LEN];
+    static uint32_t counter; /* simple counter to guarantee uniqueness */
+
+    /* if node_name is NULL, auto-generate a node name */
     if (!node_name) {
-        return;
+        int len;
+        snprintf(gen_node_name, GEN_NODE_NAME_MAX_LEN,
+                 "%s%08x", GEN_NODE_NAME_PREFIX, counter++);
+        len = strlen(gen_node_name);
+        while (len < GEN_NODE_NAME_MAX_LEN - 1) {
+            gen_node_name[len++] = g_random_int_range('A', 'Z');
+        }
+        gen_node_name[GEN_NODE_NAME_MAX_LEN - 1] = '\0';
+        node_name = gen_node_name;
     }
 
     /* empty string node name is invalid */

Signature · 1 line

1.9.3

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Quoted 26 lines · Jeff Cody

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Quoted 5 lines

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Who is this feature for?

Human users: they'll need to read through query-named-block-nodes output to find the nodes they care about. This is pretty cumbersome and not human-friendly.

Management tools: parsing query-named-block-nodes isn't trivial since the output can vary between QEMU versions (e.g. when we move I/O throttling to a block driver node there will be new internal nodes). Tools doing this should really use blockdev-add instead and assign their own node names.

It seems like neither type of user will get much mileage out of this feature. Is it really necessary or did I miss a use case?

Quoted 7 lines

Quoted 6 lines

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

Quoted 6 lines

Who is this feature for?

Human users: they'll need to read through query-named-block-nodes output to find the nodes they care about. This is pretty cumbersome and not human-friendly.

Currently, that is how a human user would find the node-names. That doesn't mean there might not be a new interface later on, that is more human friendly.

And while a human parsing query-named-block-nodes isn't fun, I think it is easier than a human assigning node-names to a graph, so it is more human-friendly than the current system.

Quoted 6 lines

Management tools: parsing query-named-block-nodes isn't trivial since the output can vary between QEMU versions (e.g. when we move I/O throttling to a block driver node there will be new internal nodes). Tools doing this should really use blockdev-add instead and assign their own node names.

Libvirt (and OpenStack) is already testing with these patches, and my impression from Eric is that parsing the output of query-named-block-nodes was less work than assigning node-names in libvirt.

Can you expand a bit on moving i/o throttle to a block, and creating new internal nodes?

The generated node-names have the same life cycle as a specified node-name; anything that would invalidate a generated node-name would invalidate a specified node-name as well.

And if a QMP command is issued that would cause new nodes to be assigned, I believe libvirt is aware that they need to perform a query-named-block-nodes again.

Quoted 5 lines

It seems like neither type of user will get much mileage out of this feature. Is it really necessary or did I miss a use case?

Strictly speaking, it isn't required. But it makes sense for QEMU to assign node-names to any unassigned node-names, because it does make life easier for both humans and management software, and QEMU is the only one that can always ensure that every BDS has a node-name.

It is also nice for QEMU; we can now in future versions assume that every BDS will always have a node-name, regardless if it has been assigned by the user or not.

And the usage of the node-names is strictly optional by the human or management software user; neither is required to use the generated node-names, and are feel to specify their own node-name. A user specified node-name will prevent an auto-generated one from being assigned for that specific BDS.

Quoted 33 lines · Stefan Hajnoczi

Quoted 26 lines · Jeff Cody

Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.

If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.

If a node name is specified, then it will not be automatically generated for that BDS entry.

If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:

    __qemu##00000000IAIYNXXR
    __qemu##00000002METXTRBQ
    __qemu##00000001FMBORDWG

The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.

Quoted 5 lines

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Quoted 6 lines

Who is this feature for?

Human users: they'll need to read through query-named-block-nodes output to find the nodes they care about. This is pretty cumbersome and not human-friendly.

Currently, that is how a human user would find the node-names. That doesn't mean there might not be a new interface later on, that is more human friendly.

And while a human parsing query-named-block-nodes isn't fun, I think it is easier than a human assigning node-names to a graph, so it is more human-friendly than the current system.

Quoted 5 lines

Management tools: parsing query-named-block-nodes isn't trivial since the output can vary between QEMU versions (e.g. when we move I/O throttling to a block driver node there will be new internal nodes). Tools doing this should really use blockdev-add instead and assign their own node names.

Libvirt (and OpenStack) is already testing with these patches, and my impression from Eric is that parsing the output of query-named-block-nodes was less work than assigning node-names in libvirt.

Can you expand a bit on moving i/o throttle to a block, and creating new internal nodes?

Quoted 4 lines · Stefan Hajnoczi

On Tue, Jun 17, 2014 at 05:53:49PM -0400, Jeff Cody wrote:
It seems like neither type of user will get much mileage out of this feature. Is it really necessary or did I miss a use case?

Strictly speaking, it isn't required. But it makes sense for QEMU to assign node-names to any unassigned node-names, because it does make life easier for both humans and management software, and QEMU is the only one that can always ensure that every BDS has a node-name.

It is also nice for QEMU; we can now in future versions assume that every BDS will always have a node-name, regardless if it has been assigned by the user or not.

And the usage of the node-names is strictly optional by the human or management software user; neither is required to use the generated node-names, and are feel to specify their own node-name. A user specified node-name will prevent an auto-generated one from being assigned for that specific BDS.

This is a small helper function, to determine if 'base' is in the chain of BlockDriverState 'top'. It returns true if it is in the chain, and false otherwise.

If either argument is NULL, it will also return false.

Reviewed-by Benoit Canet <[email protected]>

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c               | 11 +++++++++++
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+)

This is a small helper function, to determine if 'base' is in the chain of BlockDriverState 'top'. It returns true if it is in the chain, and false otherwise.

If either argument is NULL, it will also return false.

Reviewed-by Benoit Canet <[email protected]>

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c               | 11 +++++++++++
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+)

block.c +11 -0

index da32bb0..280a167 100644
@@ -3819,6 +3819,17 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
     return NULL;
 }
 
+/* If 'base' is in the same chain as 'top', return true. Otherwise,
+ * return false.  If either argument is NULL, return false. */
+bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
+{
+    while (top && top != base) {
+        top = top->backing_hd;
+    }
+
+    return top != NULL;
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {

include/block/block.h +1 -0

index f15b99b..c60bd52 100644
@@ -403,6 +403,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);
 BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
+bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 void bdrv_iterate(void (*it)(void *opaque, BlockDriverState *bs),
                   void *opaque);

Signature · 1 line

1.9.3

This simplifies the function bdrv_find_overlay(). With this change, bdrv_find_base() is just a subset of usage of bdrv_find_overlay(), so this also takes advantage of that.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

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

---
 block.c | 45 ++++++++++-----------------------------------
 1 file changed, 10 insertions(+), 35 deletions(-)

Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.

Change it to optional, with the default being the active layer in the device chain.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

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

---

Quoted 10 lines · Jeff Cody

Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.

Change it to optional, with the default being the active layer in the device chain.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

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

---

As mentioned elsewhere, this is the patch that changed from v5. But so there's no confusion, I've gone back over this one, and my R-b still stands.

blockdev.c void qmp_block_commit(const char *device, · line 1940

Quoted 8 lines

blockdev.c +5 -0

+    /* Important Note:
+     *  libvirt relies on the DeviceNotFound error class in order to probe for
+     *  live commit feature versions; for this to work, we must make sure to
+     *  perform the device lookup before any generic errors that may occur in a
+     *  scenario in which all optional arguments are omitted. */
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);

Thanks for adding the comment :)

Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.

Change it to optional, with the default being the active layer in the device chain.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

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

---
 blockdev.c             | 16 ++++++++++++++--
 qapi/block-core.json   |  7 ++++---
 qmp-commands.hx        |  5 +++--
 tests/qemu-iotests/040 | 28 ++++++++++++++++++----------
 4 files changed, 39 insertions(+), 17 deletions(-)

This modifies the block operation block-commit so that it will accept node-name arguments for either 'top' or 'base' BDS.

The filename and node-name are mutually exclusive to each other; i.e.:
"top" and "top-node-name" are mutually exclusive (enforced) "base" and "base-node-name" are mutually exclusive (enforced)

The preferred and recommended method now of using block-commit is to specify the BDS to operate on via their node-name arguments.

This also adds an explicit check that base_bs is in the chain of top_bs, because if they are referenced by their unique node-name arguments, the discovery process does not intrinsically verify they are in the same chain.

Reviewed-by Eric Blake <[email protected]>

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

---
 blockdev.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++--------
 qapi/block-core.json | 37 +++++++++++++++++++++++++----------
 qmp-commands.hx      | 31 ++++++++++++++++++++++--------
 3 files changed, 96 insertions(+), 26 deletions(-)

blockdev.c +46 -8

index a9a3b2f..44f0cc4 100644
@@ -1911,12 +1911,15 @@ void qmp_block_stream(const char *device, bool has_base,
 
 void qmp_block_commit(const char *device,
                       bool has_base, const char *base,
+                      bool has_base_node_name, const char *base_node_name,
                       bool has_top, const char *top,
+                      bool has_top_node_name, const char *top_node_name,
                       bool has_speed, int64_t speed,
                       Error **errp)
 {
-    BlockDriverState *bs;
-    BlockDriverState *base_bs, *top_bs;
+    BlockDriverState *bs = NULL;
+    BlockDriverState *base_bs = NULL;
+    BlockDriverState *top_bs = NULL;
     Error *local_err = NULL;
     /* This will be part of the QMP command, if/when the
      * BlockdevOnError change for blkmirror makes it in

@@ -1930,6 +1933,16 @@ void qmp_block_commit(const char *device,
     /* drain all i/o before commits */
     bdrv_drain_all();
 
+    /* argument combination validation */
+    if (has_base && has_base_node_name) {
+        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
+        return;
+    }
+    if (has_top && has_top_node_name) {
+        error_setg(errp, "'top' and 'top-node-name' are mutually exclusive");
+        return;
+    }
+
     /* Important Note:
      *  libvirt relies on the DeviceNotFound error class in order to probe for
      *  live commit feature versions; for this to work, we must make sure to

@@ -1941,14 +1954,16 @@ void qmp_block_commit(const char *device,
         return;
     }
 
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
-        return;
-    }
-
     /* default top_bs is the active layer */
     top_bs = bs;
 
-    if (has_top && top) {
+    if (has_top_node_name) {
+        top_bs = bdrv_lookup_bs(NULL, top_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    } else if (has_top && top) {
         if (strcmp(bs->filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }

@@ -1959,7 +1974,13 @@ void qmp_block_commit(const char *device,
         return;
     }
 
-    if (has_base && base) {
+    if (has_base_node_name) {
+        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    } else if (has_base && base) {
         base_bs = bdrv_find_backing_image(top_bs, base);
     } else {
         base_bs = bdrv_find_base(top_bs);

@@ -1976,6 +1997,23 @@ void qmp_block_commit(const char *device,
         return;
     }
 
+    /* Verify that 'base' is in the same chain as 'top' */
+    if (!bdrv_chain_contains(top_bs, base_bs)) {
+        error_setg(errp, "'base' and 'top' are not in the same chain");
+        return;
+    }
+
+    /* This should technically be caught in commit_start, but
+     * check here for validation completeness */
+    if (!bdrv_chain_contains(bs, top_bs)) {
+        error_setg(errp, "'%s' and 'top' are not in the same chain", device);
+        return;
+    }
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) {
+        return;
+    }
+
     if (top_bs == bs) {
         commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
                             bs, &local_err);

qapi/block-core.json +27 -10

index 6ddce4f..dddaa13 100644
@@ -685,14 +685,29 @@
 # Live commit of data from overlay image nodes into backing nodes - i.e.,
 # writes data between 'top' and 'base' into 'base'.
 #
-# @device:  the name of the device
+# @device:                 The name of the device.
 #
-# @base:   #optional The file name of the backing image to write data into.
-#                    If not specified, this is the deepest backing image
+# For 'base', either @base or @base-node-name may be set but not both. If
+# neither is specified, this is the deepest backing image.
 #
-# @top:    #optional The file name of the backing image within the image chain,
-#                    which contains the topmost data to be committed down. If
-#                    not specified, this is the active layer.
+# @base:          #optional The file name of the backing image to write data
+#                           into.
+#
+# @base-node-name #optional The name of the block driver state node of the
+#                           backing image to write data into.
+#                           (Since 2.1)
+#
+# For 'top', either @top or @top-node-name must be set but not both. If
+# neither is specified, this is the active layer.
+#
+# @top:           #optional The file name of the backing image within the image
+#                           chain, which contains the topmost data to be
+#                           committed down.
+#
+# @top-node-name: #optional The block driver state node name of the backing
+#                           image within the image chain, which contains the
+#                           topmost data to be committed down.
+#                           (Since 2.1)
 #
 #                    If top == base, that is an error.
 #                    If top == active, the job will not be completed by itself,

@@ -711,17 +726,19 @@
 #
 # Returns: Nothing on success
 #          If commit or stream is already active on this device, DeviceInUse
-#          If @device does not exist, DeviceNotFound
+#          If @device does not exist or cannot be determined, DeviceNotFound
 #          If image commit is not supported by this device, NotSupported
-#          If @base or @top is invalid, a generic error is returned
+#          If @base, @top, @base-node-name, @top-node-name invalid, GenericError
 #          If @speed is invalid, InvalidParameter
+#          If both @base and @base-node-name are specified, GenericError
+#          If both @top and @top-node-name are specified, GenericError
 #
 # Since: 1.3
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
-            '*speed': 'int' } }
+  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
+            '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } }
 
 ##
 # @drive-backup

qmp-commands.hx +23 -8

index 4a8e71a..52af928 100644
@@ -985,7 +985,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s?,speed:o?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 

@@ -998,13 +998,28 @@ data between 'top' and 'base' into 'base'.
 
 Arguments:
 
-- "device": The device's ID, must be unique (json-string)
-- "base": The file name of the backing image to write data into.
-          If not specified, this is the deepest backing image
-          (json-string, optional)
-- "top":  The file name of the backing image within the image chain,
-          which contains the topmost data to be committed down. If
-          not specified, this is the active layer. (json-string, optional)
+- "device":         The device's ID, must be unique (json-string)
+
+For 'base', either base or base-node-name may be set but not both. If
+neither is specified, this is the deepest backing image
+
+- "base":           The file name of the backing image to write data into.
+                    (json-string, optional)
+- "base-node-name": The name of the block driver state node of the
+                    backing image to write data into.
+                    (json-string, optional) (Since 2.1)
+
+For 'top', either top or top-node-name must be set but not both. If
+neither is specified, this is the active layer
+
+- "top":            The file name of the backing image within the image chain,
+                    which contains the topmost data to be committed down.
+                    (json-string, optional)
+
+- "top-node-name":  The block driver state node name of the backing
+                    image within the image chain, which contains the
+                    topmost data to be committed down.
+                    (json-string, optional) (Since 2.1)
 
           If top == base, that is an error.
           If top == active, the job will not be completed by itself,

Signature · 1 line

1.9.3

On some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block commit.

For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.

In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.

With this extension to the block-commit api, the user is able to change the backing file of the overlay image as part of the block-commit operation.

This allows the change to be 'safe', in the sense that if the attempt to write the overlay image metadata fails, then the block-commit operation returns failure, without disrupting the guest.

If the commit top is the active layer, then specifying the backing file string will be treated as an error (there is no overlay image to modify in that case).

If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.

Reviewed-by Eric Blake <[email protected]>

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

---
 block.c                   |  8 ++++++--
 block/commit.c            |  9 ++++++---
 blockdev.c                |  8 +++++++-
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qapi/block-core.json      | 23 +++++++++++++++++++++--
 qmp-commands.hx           | 19 ++++++++++++++++++-
 7 files changed, 62 insertions(+), 11 deletions(-)

This adds the ability for block-stream to use node-name arguments for base, to specify the backing image to stream from.

Both 'base' and 'base-node-name' are optional, but mutually exclusive. Either can be specified, but not both together.

Reviewed-by Eric Blake <[email protected]>

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

---
 blockdev.c           | 48 +++++++++++++++++++++++++++++++++++++++---------
 hmp.c                |  2 +-
 qapi/block-core.json | 16 ++++++++++++----
 qmp-commands.hx      |  2 +-
 4 files changed, 53 insertions(+), 15 deletions(-)

blockdev.c +39 -9

index 7a1c966..2926738 100644
@@ -1868,38 +1868,68 @@ static void block_job_cb(void *opaque, int ret)
     bdrv_put_ref_bh_schedule(bs);
 }
 
-void qmp_block_stream(const char *device, bool has_base,
-                      const char *base, bool has_speed, int64_t speed,
+void qmp_block_stream(const char *device,
+                      bool has_base, const char *base,
+                      bool has_base_node_name, const char *base_node_name,
+                      bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     BlockDriverState *base_bs = NULL;
+    BlockDriverState *tmp_bs;
     Error *local_err = NULL;
+    const char *base_name = NULL;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
+    if (has_base && has_base_node_name) {
+        error_setg(errp, "'base' and 'base-node-name' are mutually exclusive");
+        return;
+    }
+
     bs = bdrv_find(device);
     if (!bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
+    if (has_base_node_name) {
+        base_bs = bdrv_lookup_bs(NULL, base_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+        tmp_bs = bdrv_find_overlay(bs, base_bs);
+        if (tmp_bs) {
+            base_name = tmp_bs->backing_file;
+        }
+    }
+
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         return;
     }
 
-    if (base) {
+    if (has_base) {
         base_bs = bdrv_find_backing_image(bs, base);
-        if (base_bs == NULL) {
-            error_set(errp, QERR_BASE_NOT_FOUND, base);
-            return;
-        }
+        base_name = base;
+    }
+
+    if (base_bs == NULL && (has_base || has_base_node_name)) {
+        error_set(errp, QERR_BASE_NOT_FOUND, base);
+        return;
+    }
+
+    /* Verify that 'base' is in the same chain as 'bs', if 'base' was
+     * specified */
+    if (base_bs && !bdrv_chain_contains(bs, base_bs)) {
+        error_setg(errp, "'device' and 'base' are not in the same chain");
+        return;
     }
 
-    stream_start(bs, base_bs, base, has_speed ? speed : 0,
+    stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
                  on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);

hmp.c +1 -1

index ccc35d4..bb934df 100644
@@ -1173,7 +1173,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(device, base != NULL, base,
-                     qdict_haskey(qdict, "speed"), speed,
+                     false, NULL, qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
     hmp_handle_error(mon, &error);

qapi/block-core.json +12 -4

index ae1dde9..d17e349 100644
@@ -904,9 +904,17 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @device:                  The device name.
+#
+# For 'base', either @base or @base-node-name may be set but not both. If
+# neither is specified, the entire chain will be streamed into the active image,
+# and the chain will consist of a single image (the current active layer) with
+# no backing file.
 #
-# @base:   #optional the common backing file name
+# @base:           #optional the common backing file name
+#
+# @base-node-name: #optional the block driver state node name of the
+#                            common backing file.  (Since 2.1)
 #
 # @speed:  #optional the maximum speed, in bytes per second
 #

@@ -920,8 +928,8 @@
 # Since: 1.1
 ##
 { 'command': 'block-stream',
-  'data': { 'device': 'str', '*base': 'str', '*speed': 'int',
-            '*on-error': 'BlockdevOnError' } }
+  'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str',
+            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:

qmp-commands.hx +1 -1

index 7eb90a9..41e3853 100644
@@ -979,7 +979,7 @@ EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?,speed:o?,on-error:s?",
+        .args_type  = "device:B,base:s?,base-node-name:s?,speed:o?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_block_stream,
     },
 

Signature · 1 line

1.9.3

This adds the ability for block-stream to use node-name arguments for base, to specify the backing image to stream from.

Both 'base' and 'base-node-name' are optional, but mutually exclusive. Either can be specified, but not both together.

Reviewed-by Eric Blake <[email protected]>

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

---
 blockdev.c           | 48 +++++++++++++++++++++++++++++++++++++++---------
 hmp.c                |  2 +-
 qapi/block-core.json | 16 ++++++++++++----
 qmp-commands.hx      |  2 +-
 4 files changed, 53 insertions(+), 15 deletions(-)

On some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block job.

For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.

In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.

With this extension to the block-stream api, the user is able to change the backing file of the active layer as part of the block-stream operation.

This allows the change to be 'safe', in the sense that if the attempt to write the active image metadata fails, then the block-stream operation returns failure, without disrupting the guest.

If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.

Reviewed-by Eric Blake <[email protected]>

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

---
 block/stream.c       | 11 +++++------
 blockdev.c           | 12 ++++++++++++
 hmp.c                |  3 ++-
 qapi/block-core.json | 18 +++++++++++++++++-
 qmp-commands.hx      |  2 +-
 5 files changed, 37 insertions(+), 9 deletions(-)

The QMP command 'block-stream' was missing QMP documentation. Add that documentation.

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Benoit Canet <[email protected]>

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

---
 qmp-commands.hx | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

This allows a user to make a live change to the backing file recorded in an open image.

The image file to modify can be specified 2 ways:

1) image filename
2) image node-name

Note: this does not cause the backing file itself to be reopened; it merely changes the backing filename in the image file structure, and in internal BDS structures.

It is the responsibility of the user to pass a filename string that can be resolved when the image chain is reopened, and the filename string is not validated.

A good analogy for this command is that it is a live version of 'qemu-img rebase -u', with respect to changing the backing file string.

Reviewed-by Eric Blake <[email protected]>

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

---
 blockdev.c           | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  60 ++++++++++++++++++++++++++++++
 qmp-commands.hx      |  74 +++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)

blockdev.c +102 -0

index 42c7b3f..273e52a 100644
@@ -2414,6 +2414,108 @@ void qmp_block_job_complete(const char *device, Error **errp)
     block_job_complete(job, errp);
 }
 
+void qmp_change_backing_file(const char *device,
+                             bool has_image, const char *image,
+                             bool has_image_node_name,
+                             const char *image_node_name,
+                             const char *backing_file,
+                             Error **errp)
+{
+    BlockDriverState *bs = NULL;
+    BlockDriverState *image_bs = NULL;
+    Error *local_err = NULL;
+    bool ro;
+    int open_flags;
+    int ret;
+
+    /* validate argument combinations */
+    if (has_image && has_image_node_name) {
+        error_setg(errp, "'image' and 'image-node-name' "
+                         "are mutually exclusive");
+        return;
+    }
+
+    /* find the top layer BDS of the chain */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (has_image_node_name) {
+        image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    if (has_image) {
+        if (!strcmp(bs->filename, image)) {
+            image_bs = bs;
+        } else {
+            image_bs = bdrv_find_backing_image(bs, image);
+        }
+    }
+
+    if (!has_image && !has_image_node_name) {
+        image_bs = bs;
+    }
+
+    if (!image_bs) {
+        error_setg(errp, "image file not found");
+        return;
+    }
+
+    if (bdrv_find_base(image_bs) == image_bs) {
+        error_setg(errp, "not allowing backing file change on an image "
+                         "without a backing file");
+        return;
+    }
+
+    /* even though we are not necessarily operating on bs, we need it to
+     * determine if block ops are currently prohibited on the chain */
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
+        return;
+    }
+
+    /* final sanity check */
+    if (!bdrv_chain_contains(bs, image_bs)) {
+        error_setg(errp, "'%s' and image file are not in the same chain",
+                   device);
+        return;
+    }
+
+    /* if not r/w, reopen to make r/w */
+    open_flags = image_bs->open_flags;
+    ro = bdrv_is_read_only(image_bs);
+
+    if (ro) {
+        bdrv_reopen(image_bs, open_flags | BDRV_O_RDWR, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    ret = bdrv_change_backing_file(image_bs, backing_file,
+                               image_bs->drv ? image_bs->drv->format_name : "");
+
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not change backing file to '%s'",
+                         backing_file);
+        /* don't exit here, so we can try to restore open flags if
+         * appropriate */
+    }
+
+    if (ro) {
+        bdrv_reopen(image_bs, open_flags, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err); /* will preserve prior errp */
+        }
+    }
+}
+
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
     QmpOutputVisitor *ov = qmp_output_visitor_new();

qapi/block-core.json +60 -0

index c76d45d..0939143 100644
@@ -680,6 +680,66 @@
   'data': 'BlockdevSnapshot' }
 
 ##
+# @change-backing-file
+#
+# Change the backing file in the image file metadata.  This does not cause QEMU
+# to reopen the image file to reparse the backing filename (it may, however,
+# perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
+# The new backing file string is written into the image file metadata, and the
+# QEMU internal strings are updated.
+#
+# The image file to perform the operation on can be specified by two different
+# methods:
+#
+#  Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
+#            filename.  This would use arguments @device and @image.
+#
+#  Method 2: Supply the device name, and the node-name of the image to modify,
+#            via @image-node-name.
+#
+# Arguments @image and @image-node-name are mutually exclusive.
+#
+# Method 1 interface
+#---------------------
+# @image:          #optional The file name of the image to modify.  If omitted,
+#                            and @image-node-name is not supplied, then the
+#                            default is the active layer of the chain described
+#                            by @device.
+#
+# Method 2 interface
+#---------------------
+# @image-node-name #optional The name of the block driver state node of the
+#                            image to modify.  The @device argument is used to
+#                            verify @image-node-name is in the chain described
+#                            by @device.
+#
+# Common arguments
+#---------------------
+# @device:          The name of the device.
+#
+# @backing-file:    The string to write as the backing file.  This string is
+#                   not validated, so care should be taken when specifying
+#                   the string or the image chain may not be able to be
+#                   reopened again.
+#
+#                   If a pathname string is such that it cannot be
+#                   resolved by QEMU, that means that subsequent QMP or
+#                   HMP commands must use node-names for the image in
+#                   question, as filename lookup methods will fail.
+#
+#
+# Returns: Nothing on success
+#          If @device does not exist or cannot be determined, DeviceNotFound
+#          If @image is specified, but not @device, GenericError
+#          If both @image and @image-node-name are specified, GenericError
+#
+# Since: 2.1
+##
+{ 'command': 'change-backing-file',
+  'data': { 'device': 'str', '*image': 'str', '*image-node-name': 'str',
+            'backing-file': 'str' } }
+
+##
 # @block-commit
 #
 # Live commit of data from overlay image nodes into backing nodes - i.e.,

qmp-commands.hx +74 -0

index 314cbba..c1cbfd2 100644
@@ -1438,6 +1438,80 @@ Example:
 EQMP
 
     {
+        .name       = "change-backing-file",
+        .args_type  = "device:s,image:s?,image-node-name:s?,backing-file:s",
+        .mhandler.cmd_new = qmp_marshal_input_change_backing_file,
+    },
+
+SQMP
+change-backing-file
+-------------------
+Since: 2.1
+
+Change the backing file in the image file metadata.  This does not cause QEMU
+to reopen the image file to reparse the backing filename (it may, however,
+perform a reopen to change permissions from r/o -> r/w -> r/o, if needed).
+The new backing file string is written into the image file metadata, and the
+QEMU internal strings are updated.
+
+The image file to perform the operation on can be specified by two different
+methods:
+
+ Method 1: Supply the device name (e.g. 'virtio0'), and optionally the image
+           filename.  This would use arguments "device" and "image".
+
+ Method 2: Supply the device name, and the node-name of the image to modify,
+           via "image-node-name".
+
+Arguments:
+
+Arguments "image" or "image-node-name" are mutually exclusive.
+
+
+Method 1 interface
+--------------------
+- "image":              The file name of the image to modify.  If omitted,
+                        and "image-node-name" is not supplied, then the
+                        default is the active layer of the chain described
+                        by device.
+                        (json-string, optional)
+
+
+Method 2 interface
+--------------------
+- "image-node-name":    The name of the block driver state node of the
+                        image to modify.  The "device" is argument is used to
+                        verify "image-node-name" is in the chain described by
+                        "device".
+                        (json-string, optional)
+
+
+Common arguments
+--------------------
+- "device":             The name of the device.
+                        (json-string)
+
+- "backing-file":       The string to write as the backing file.  This string is
+                        not validated, so care should be taken when specifying
+                        the string or the image chain may not be able to be
+                        reopened again.
+                        (json-string)
+
+                        If a pathname string is such that it cannot be
+                        resolved by QEMU, that means that subsequent QMP or
+                        HMP commands must use node-names for the image in
+                        question, as filename lookup methods will fail.
+
+
+Returns: Nothing on success
+         If "device" does not exist or cannot be determined, DeviceNotFound
+         If "image" is specified, but not "device, GenericError
+         If both "image" and "image-node-name" are specified, GenericError
+
+
+EQMP
+
+    {
         .name       = "balloon",
         .args_type  = "value:M",
         .mhandler.cmd_new = qmp_marshal_input_balloon,

Signature · 1 line

1.9.3

Could you provide an example script of how these backing file manipulation are suposed to be done ?

Quoted 2 lines · Benoît Canet

Could you provide an example script of how these backing file manipulation are suposed to be done ?

I agree.

Jeff: Please send a qemu-iotests test case.

This allows a user to make a live change to the backing file recorded in an open image.

The image file to modify can be specified 2 ways:

1) image filename
2) image node-name

Note: this does not cause the backing file itself to be reopened; it merely changes the backing filename in the image file structure, and in internal BDS structures.

It is the responsibility of the user to pass a filename string that can be resolved when the image chain is reopened, and the filename string is not validated.

A good analogy for this command is that it is a live version of 'qemu-img rebase -u', with respect to changing the backing file string.

Reviewed-by Eric Blake <[email protected]>

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

---
 blockdev.c           | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json |  60 ++++++++++++++++++++++++++++++
 qmp-commands.hx      |  74 +++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)

Changes from v5->v6:

* Check for attempt to commit an image to itself (Eric)
* Add a comment to the bdrv_find for block-commit, indicating that libvirt uses the error case for probing (Eric)
* Added Benoit's R-b's

Changes from v4->v5:

* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's

Changes from v3->v4:

* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above, so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6 Added Eric's Reviewed-by: on patches 8,9

Changes from v2->v3:

* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream

Using node-names instead of filenames for block job operations over QMP is a superior method of identifying the block driver images to operate on, as it removes all pathname ambiguity.

This series modifies block-commit and block-stream to use node-names, and creates a new QAPI command to allow stand-alone backing file changes on an image file.

So that node-names can be used as desired for all block job operations, this series also auto-generates node-names for every BDS. User-specified node-names will override any autogenerated

Jeff Cody (10):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
block: simplify bdrv_find_base() and bdrv_find_overlay()
block: make 'top' argument to block-commit optional
block: Accept node-name arguments for block-commit
block: extend block-commit to accept a string for the backing file
block: add ability for block-stream to use node-name
block: add backing-file option to block-stream
block: Add QMP documentation for block-stream
block: add QAPI command to allow live backing file change

 block.c                   |  80 ++++++++--------
 block/commit.c            |   9 +-
 block/stream.c            |  11 +--
 blockdev.c                | 238 ++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |   1 +
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      | 145 +++++++++++++++++++++++++---
 qmp-commands.hx           | 181 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 602 insertions(+), 98 deletions(-)

Quoted 59 lines · Jeff Cody

Changes from v5->v6:

* Check for attempt to commit an image to itself (Eric)
* Add a comment to the bdrv_find for block-commit, indicating that libvirt uses the error case for probing (Eric)
* Added Benoit's R-b's

Changes from v4->v5:

* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's

Changes from v3->v4:

* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above, so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6 Added Eric's Reviewed-by: on patches 8,9

Changes from v2->v3:

* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream

Using node-names instead of filenames for block job operations over QMP is a superior method of identifying the block driver images to operate on, as it removes all pathname ambiguity.

This series modifies block-commit and block-stream to use node-names, and creates a new QAPI command to allow stand-alone backing file changes on an image file.

So that node-names can be used as desired for all block job operations, this series also auto-generates node-names for every BDS. User-specified node-names will override any autogenerated

Jeff Cody (10):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
block: simplify bdrv_find_base() and bdrv_find_overlay()
block: make 'top' argument to block-commit optional
block: Accept node-name arguments for block-commit
block: extend block-commit to accept a string for the backing file
block: add ability for block-stream to use node-name
block: add backing-file option to block-stream
block: Add QMP documentation for block-stream
block: add QAPI command to allow live backing file change

Quoted 11 lines

 block.c                   |  80 ++++++++--------
 block/commit.c            |   9 +-
 block/stream.c            |  11 +--
 blockdev.c                | 238 ++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |   1 +
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      | 145 +++++++++++++++++++++++++---
 qmp-commands.hx           | 181 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 602 insertions(+), 98 deletions(-)

This series side-steps lack of child op blockers by checking only the root node/drive.

Existing node-name commands like resize and snapshot-sync check for op blockers on the actual node. They do not take the same approach as this patch series.

We have a mess and I don't want to commit this series before we've figured out what to do about child op blockers.

Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

c. We're painting ourselves into a corner by using the root node for op
blocker checks. We'll have to apply the same op blockers to all nodes in a graph. There's no opportunity to apply different op blockers to a subset of the child nodes. I *think* this can be changed later without affecting the QMP API, so it's not a critical issue.

The answer seems to be that op blockers should be propagated to all child nodes and commands should test the node, not the drive/root node. That gives us the flexibility for per-node op blockers in the future and maintains compatibility with existing node-name users.

Having said that, to be fair, the new QAPI command change-backing-file does propagate this top-layer in-use flag semantic, but I would prefer that patch to be dropped rather than not committing this series.

Quoted 9 lines

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

The ability for internal BDSs to share a common base BDS makes some block jobs unsafe currently, I believe. A crude and ugly fix is to only allow a single block-job to occur at any given time, but that doesn't seem feasible, so let's ignore that.

Perhaps, for 2.1, provide an overlay pointer list inside each BDS (some of my earlier patches in this series had a single overlay, but that is not enough). We could then apply op blockers to the topmost nodes for any affected BDS image in a chain, by navigating upwards. Not sure how complex this would be in practice, though.

We could also apply child blockers to all nodes in all directions in a graph, if we don't want to rely on the topmost image as a blocker proxy for the whole drive.

Quoted 7 lines

c. We're painting ourselves into a corner by using the root node for op
blocker checks. We'll have to apply the same op blockers to all nodes in a graph. There's no opportunity to apply different op blockers to a subset of the child nodes. I *think* this can be changed later without affecting the QMP API, so it's not a critical issue.

We've already painted ourselves in that corner, alas.

I agree that from a QAPI perspective, the change is not critical:
once op blockers are correctly applied to all child nodes, any API change (e.g. commit or stream) would likely be optional only (such as, making 'device' optional instead of mandatory), and thus discoverable.

Quoted 7 lines

The answer seems to be that op blockers should be propagated to all child nodes and commands should test the node, not the drive/root node. That gives us the flexibility for per-node op blockers in the future and maintains compatibility with existing node-name users.

Long term thoughts:

So if I think of operations that are done on block devices from a block job, and chuck them into categories, I think we have:

1) Read of guest-visible data
2) Write of guest-visible data
3) Read of host-visible data (e.g. image file metadata)
4) Write of host-visible data (e.g. image file metadata, such as the backing-file)
5) Block chain manipulations (e.g. movement of a BDS, change to r/w instead of r/o, etc..)
6) I/O attribute changes (e.g. throttling, etc..)

Does this make sense, and did I miss any (likely so)? It seems like we should issue blockers not based on specific commands (e.g. BLOCK_OP_TYPE_COMMIT), but rather based on what specific category of interaction on a BDS we want to prohibit / allow.

I don't think specific command blockers provide enough granularity, and doesn't necessarily scale well as new commands are added. It forces a new block job author to go through the specific implementation of the other block job commands, and interpret what operations to prohibit based on what other jobs do. Whereas if each command issues blockers based on the operation category, it takes care of itself, and I just issue blockers based on my block job behavior.

Each command would then issue appropriate operational blockers to each BDS affected by an operation. For instance, at first blush, I think block-commit would want (at the very least) to block (and check) the following, in this example chain:

     [base] <-- [int1] <--  [int2] <-- [int3] <-- [top] <-- [overlay]

     becomes:

     [base] <-- [overlay]

Blocked operations per image:

* 'base' image

Operation | Blocked
-----------------------------

GUEST READ      |   Y
GUEST WRITE     |   Y
HOST READ       |    
HOST WRITE      |    
CHAIN           |   Y
I/O ATTRIBUTE   |

* Intermediate images between 'base' up to and including 'top':

Operation | Blocked
-----------------------------

GUEST READ      |   
GUEST WRITE     |   Y
HOST READ       |
HOST WRITE      |   Y
CHAIN           |   Y
I/O ATTRIBUTE   |

* The overlay of 'top', if it exists:

Operation | Blocked
-----------------------------

GUEST READ      |   
GUEST WRITE     |    
HOST READ       |
HOST WRITE      |   Y
CHAIN           |   Y
I/O ATTRIBUTE   |

if qemu allows shared BDS, and I am able to open 'base' as a shared BDS for both disks, then an attempt to block-commit img1 into base will be blocked because base is still in use by img2. But as a management app, I shouldn't be attempting that in the first place. Yes, it would be nice if qemu blocks me from being stupid; but even if qemu allows shared BDS, if I don't open a shared BDS on 'base' in the first place, it is no different than if qemu doesn't allow shared BDS. Either way, it's not qemu's fault if I, as management, try to commit into a file that is in use by more than one chain.

if I am not able to do a shared BDS, I'm still
and as the management, I did not request that qemu open 'base' as a shared BDS between both

On 06/19/2014 10:26 AM, Jeff Cody wrote:

Quoted 15 lines

Long term thoughts:

So if I think of operations that are done on block devices from a block job, and chuck them into categories, I think we have:

1) Read of guest-visible data
2) Write of guest-visible data
3) Read of host-visible data (e.g. image file metadata)
4) Write of host-visible data (e.g. image file metadata, such as the backing-file)
5) Block chain manipulations (e.g. movement of a BDS, change to r/w instead of r/o, etc..)
6) I/O attribute changes (e.g. throttling, etc..)

Quoted 4 lines

Does this make sense, and did I miss any (likely so)? It seems like we should issue blockers not based on specific commands (e.g. BLOCK_OP_TYPE_COMMIT), but rather based on what specific category of interaction on a BDS we want to prohibit / allow.

Yes, I like that train of thought - it's not the operation that we are blocking, but the access category(ies) required by that operation.

Quoted 26 lines

I don't think specific command blockers provide enough granularity, and doesn't necessarily scale well as new commands are added. It forces a new block job author to go through the specific implementation of the other block job commands, and interpret what operations to prohibit based on what other jobs do. Whereas if each command issues blockers based on the operation category, it takes care of itself, and I just issue blockers based on my block job behavior.

Each command would then issue appropriate operational blockers to each BDS affected by an operation. For instance, at first blush, I think block-commit would want (at the very least) to block (and check) the following, in this example chain:

     [base] <-- [int1] <--  [int2] <-- [int3] <-- [top] <-- [overlay]

     becomes:

     [base] <-- [overlay]

Blocked operations per image:

* 'base' image

Quoted 8 lines

Operation | Blocked
-----------------------------

GUEST READ      |   Y
GUEST WRITE     |   Y
HOST READ       |    
HOST WRITE      |    
CHAIN           |   Y
I/O ATTRIBUTE   |

Makes sense:

blocking guest read because we are actively modifying the contents; if any other operation branches off of base, they will see data that is not consistent with what the guest sees through overlay.

blocking guest write because it is a backing chain, and overlay still depends on base image for sectors that have not yet been commited..

blocking chain manipulations because we are going to do a chain manipulation once our command completes.

Maybe you also want to block HOST_WRITE (our chain manipulation is done by writing host metadata; so if someone else writes that under our feet, we may have races on what gets written).

Quoted 4 lines

* Intermediate images between 'base' up to and including 'top':

Quoted 9 lines

Operation | Blocked
-----------------------------

GUEST READ      |   
GUEST WRITE     |   Y
HOST READ       |
HOST WRITE      |   Y
CHAIN           |   Y
I/O ATTRIBUTE   |

This needs to also block GUEST_READ - as soon as the commit starts modifying base, ALL intermediate images are invalid (they no longer represent a state of memory as was seen by the guest). Once you start a commit, you MUST NOT allow a fleecing operation on any of the intermediate operations.

Quoted 14 lines

* The overlay of 'top', if it exists:

Operation | Blocked
-----------------------------

GUEST READ      |   
GUEST WRITE     |    
HOST READ       |
HOST WRITE      |   Y
CHAIN           |   Y
I/O ATTRIBUTE   |

Quoted 6 lines

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Quoted 4 lines

The ability for internal BDSs to share a common base BDS makes some block jobs unsafe currently, I believe. A crude and ugly fix is to only allow a single block-job to occur at any given time, but that doesn't seem feasible, so let's ignore that.

Can we even create a common base BDS in qemu 2.1? I know that the blockdev-add command was designed with this possibility in mind, but to my knowledge, we have not actually wired it up yet.

Quoted 20 lines

Quoted 5 lines

The answer seems to be that op blockers should be propagated to all child nodes and commands should test the node, not the drive/root node. That gives us the flexibility for per-node op blockers in the future and maintains compatibility with existing node-name users.

Long term thoughts:

So if I think of operations that are done on block devices from a block job, and chuck them into categories, I think we have:

1) Read of guest-visible data
2) Write of guest-visible data
3) Read of host-visible data (e.g. image file metadata)
4) Write of host-visible data (e.g. image file metadata, such as the backing-file)
5) Block chain manipulations (e.g. movement of a BDS, change to r/w instead of r/o, etc..)
6) I/O attribute changes (e.g. throttling, etc..)

Quoted 4 lines

Does this make sense, and did I miss any (likely so)? It seems like we should issue blockers not based on specific commands (e.g. BLOCK_OP_TYPE_COMMIT), but rather based on what specific category of interaction on a BDS we want to prohibit / allow.

Yes, I like that train of thought - it's not the operation that we are blocking, but the access category(ies) required by that operation.

Quoted 9 lines · Stefan Hajnoczi

On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote:
Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

The ability for internal BDSs to share a common base BDS makes some block jobs unsafe currently, I believe. A crude and ugly fix is to only allow a single block-job to occur at any given time, but that doesn't seem feasible, so let's ignore that.

Quoted 16 lines · Jeff Cody

Quoted 9 lines · Stefan Hajnoczi

On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote:
Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Checking op blockers on the root node as a stop-gap is a good idea. Let's apply it across all commands (e.g. snapshot-sync, resize).

Fam pointed out that this approach is vulnerable to blockdev-add, where blockers could be set/checked on an incomplete BDS graph (since you can add new nodes on top). Do we need to move the blockers up the graph if a new root node is inserted?

Besides this issue, your approach seems like the quickest safe solution for 2.1.

Quoted 5 lines

The ability for internal BDSs to share a common base BDS makes some block jobs unsafe currently, I believe. A crude and ugly fix is to only allow a single block-job to occur at any given time, but that doesn't seem feasible, so let's ignore that.

Right now I don't think we share BDS chains.

Stefan

Quoted 16 lines · Jeff Cody

Quoted 9 lines · Stefan Hajnoczi

On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote:
Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Checking op blockers on the root node as a stop-gap is a good idea. Let's apply it across all commands (e.g. snapshot-sync, resize).

Fam pointed out that this approach is vulnerable to blockdev-add, where blockers could be set/checked on an incomplete BDS graph (since you can add new nodes on top). Do we need to move the blockers up the graph if a new root node is inserted?

Quoted 23 lines · Stefan Hajnoczi

Quoted 16 lines · Jeff Cody

Quoted 9 lines · Stefan Hajnoczi

On Tue, Jun 17, 2014 at 05:53:48PM -0400, Jeff Cody wrote:
Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Quoted 7 lines

Checking op blockers on the root node as a stop-gap is a good idea. Let's apply it across all commands (e.g. snapshot-sync, resize).

Fam pointed out that this approach is vulnerable to blockdev-add, where blockers could be set/checked on an incomplete BDS graph (since you can add new nodes on top). Do we need to move the blockers up the graph if a new root node is inserted?

My concern is if we allow adding new root on top, it's not easy to know the real root then.

To give an example:

If we have

    [base id=""] <- [active id="drive0" blockers=...]

When user does

    (QMP) block-commit device="drive0" ...

We should check drive0, which is OK.

Then, assume user adds a new root on top, we would take care of moving the blockers:

    [base id=""] <- [active id="drive0"] <- [active id="drive1" blockers=]

At this point, what if user does something on drive0 again?

    (QMP) block-commit device="drive0" ...

The right thing to do is to check blockers on "drive1", since it's the real root now. But how do we know? Do we need to add a back reference pointer ->overlap_hd in BDS, or do we maintain a look up table, or do we search all BDS graphs to figure out?

None is easier than if we put the blockers in the bottom BDS, in the first place:

    [base id="" blockers=...] <- [active id="drive0"]
                ^^^^^^^^^^^^

Even if user adds a new root, we don't need to worry about moving blockers, because the bottom is not changed.

    [base id="" blockers=...] <- [active id="drive0"] <- [active id="drive1"]

Checking the blockers are easy, either for drive0 or drive1: just follow the backing chain until getting to the end.

Fam

Quoted 61 lines · Stefan Hajnoczi

Quoted 23 lines · Jeff Cody

Quoted 16 lines · Stefan Hajnoczi

Quoted 9 lines · Jeff Cody

Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Quoted 7 lines

Checking op blockers on the root node as a stop-gap is a good idea. Let's apply it across all commands (e.g. snapshot-sync, resize).

Fam pointed out that this approach is vulnerable to blockdev-add, where blockers could be set/checked on an incomplete BDS graph (since you can add new nodes on top). Do we need to move the blockers up the graph if a new root node is inserted?

My concern is if we allow adding new root on top, it's not easy to know the real root then.

To give an example:

If we have

    [base id=""] <- [active id="drive0" blockers=...]

When user does

    (QMP) block-commit device="drive0" ...

We should check drive0, which is OK.

Then, assume user adds a new root on top, we would take care of moving the blockers:

    [base id=""] <- [active id="drive0"] <- [active id="drive1" blockers=]

At this point, what if user does something on drive0 again?

    (QMP) block-commit device="drive0" ...

The right thing to do is to check blockers on "drive1", since it's the real root now. But how do we know? Do we need to add a back reference pointer ->overlap_hd in BDS, or do we maintain a look up table, or do we search all BDS graphs to figure out?

Quoted 6 lines

None is easier than if we put the blockers in the bottom BDS, in the first place:

    [base id="" blockers=...] <- [active id="drive0"]
                ^^^^^^^^^^^^

I think you are right. If we place the blocker at the bottom-most BDS, then that would be a more restrictive blocker. This may end up being more restrictive than needed, but more importantly it should make everything safe.

Also, it is an easy change for 2.1 - just call bdrv_find_base(bs), and set/check/clear blockers on the returned BDS.

Quoted 9 lines

Even if user adds a new root, we don't need to worry about moving blockers, because the bottom is not changed.

    [base id="" blockers=...] <- [active id="drive0"] <- [active id="drive1"]

Checking the blockers are easy, either for drive0 or drive1: just follow the backing chain until getting to the end.

Fam

Am 24.06.2014 um 15:32 hat Jeff Cody geschrieben:

Quoted 74 lines · Fam Zheng

Quoted 61 lines · Stefan Hajnoczi

Quoted 23 lines · Jeff Cody

Quoted 16 lines · Stefan Hajnoczi

Quoted 9 lines · Jeff Cody

Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

Quoted 5 lines

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

Quoted 5 lines

I don't think (b) is the ultimate solution. It is used as a stop-gap because op blockers in the current implementation is essentially analogous to the in-use flag. But is it good enough for 2.1? If *everything* checks the topmost node in 2.1, then I think we are OK in all cases except where images files share a common BDS.

Quoted 7 lines

Checking op blockers on the root node as a stop-gap is a good idea. Let's apply it across all commands (e.g. snapshot-sync, resize).

Fam pointed out that this approach is vulnerable to blockdev-add, where blockers could be set/checked on an incomplete BDS graph (since you can add new nodes on top). Do we need to move the blockers up the graph if a new root node is inserted?

My concern is if we allow adding new root on top, it's not easy to know the real root then.

To give an example:

If we have

    [base id=""] <- [active id="drive0" blockers=...]

When user does

    (QMP) block-commit device="drive0" ...

We should check drive0, which is OK.

Then, assume user adds a new root on top, we would take care of moving the blockers:

    [base id=""] <- [active id="drive0"] <- [active id="drive1" blockers=]

At this point, what if user does something on drive0 again?

    (QMP) block-commit device="drive0" ...

The right thing to do is to check blockers on "drive1", since it's the real root now. But how do we know? Do we need to add a back reference pointer ->overlap_hd in BDS, or do we maintain a look up table, or do we search all BDS graphs to figure out?

Quoted 6 lines

None is easier than if we put the blockers in the bottom BDS, in the first place:

    [base id="" blockers=...] <- [active id="drive0"]
                ^^^^^^^^^^^^

I think you are right. If we place the blocker at the bottom-most BDS, then that would be a more restrictive blocker. This may end up being more restrictive than needed, but more importantly it should make everything safe.

Quoted 2 lines

Also, it is an easy change for 2.1 - just call bdrv_find_base(bs), and set/check/clear blockers on the returned BDS.

What does bdrv_find_base() return for e.g. quorum?

Kevin

Quoted 9 lines

Quoted 9 lines

Even if user adds a new root, we don't need to worry about moving blockers, because the bottom is not changed.

    [base id="" blockers=...] <- [active id="drive0"] <- [active id="drive1"]

Checking the blockers are easy, either for drive0 or drive1: just follow the backing chain until getting to the end.

Fam

Quoted 59 lines · Jeff Cody

Changes from v5->v6:

* Check for attempt to commit an image to itself (Eric)
* Add a comment to the bdrv_find for block-commit, indicating that libvirt uses the error case for probing (Eric)
* Added Benoit's R-b's

Changes from v4->v5:

* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's

Changes from v3->v4:

* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above, so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6 Added Eric's Reviewed-by: on patches 8,9

Changes from v2->v3:

* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream

Using node-names instead of filenames for block job operations over QMP is a superior method of identifying the block driver images to operate on, as it removes all pathname ambiguity.

This series modifies block-commit and block-stream to use node-names, and creates a new QAPI command to allow stand-alone backing file changes on an image file.

So that node-names can be used as desired for all block job operations, this series also auto-generates node-names for every BDS. User-specified node-names will override any autogenerated

Jeff Cody (10):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
block: simplify bdrv_find_base() and bdrv_find_overlay()
block: make 'top' argument to block-commit optional
block: Accept node-name arguments for block-commit
block: extend block-commit to accept a string for the backing file
block: add ability for block-stream to use node-name
block: add backing-file option to block-stream
block: Add QMP documentation for block-stream
block: add QAPI command to allow live backing file change

Quoted 11 lines

 block.c                   |  80 ++++++++--------
 block/commit.c            |   9 +-
 block/stream.c            |  11 +--
 blockdev.c                | 238 ++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |   1 +
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      | 145 +++++++++++++++++++++++++---
 qmp-commands.hx           | 181 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 602 insertions(+), 98 deletions(-)

This series side-steps lack of child op blockers by checking only the root node/drive.

Existing node-name commands like resize and snapshot-sync check for op blockers on the actual node. They do not take the same approach as this patch series.

We have a mess and I don't want to commit this series before we've figured out what to do about child op blockers.

Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

c. We're painting ourselves into a corner by using the root node for op
blocker checks. We'll have to apply the same op blockers to all nodes in a graph. There's no opportunity to apply different op blockers to a subset of the child nodes. I *think* this can be changed later without affecting the QMP API, so it's not a critical issue.

The answer seems to be that op blockers should be propagated to all child nodes and commands should test the node, not the drive/root node. That gives us the flexibility for per-node op blockers in the future and maintains compatibility with existing node-name users.

Stefan

Quoted 59 lines · Jeff Cody

Changes from v5->v6:

* Check for attempt to commit an image to itself (Eric)
* Add a comment to the bdrv_find for block-commit, indicating that libvirt uses the error case for probing (Eric)
* Added Benoit's R-b's

Changes from v4->v5:

* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's

Changes from v3->v4:

* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above, so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6 Added Eric's Reviewed-by: on patches 8,9

Changes from v2->v3:

* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream

Using node-names instead of filenames for block job operations over QMP is a superior method of identifying the block driver images to operate on, as it removes all pathname ambiguity.

This series modifies block-commit and block-stream to use node-names, and creates a new QAPI command to allow stand-alone backing file changes on an image file.

So that node-names can be used as desired for all block job operations, this series also auto-generates node-names for every BDS. User-specified node-names will override any autogenerated

Jeff Cody (10):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
block: simplify bdrv_find_base() and bdrv_find_overlay()
block: make 'top' argument to block-commit optional
block: Accept node-name arguments for block-commit
block: extend block-commit to accept a string for the backing file
block: add ability for block-stream to use node-name
block: add backing-file option to block-stream
block: Add QMP documentation for block-stream
block: add QAPI command to allow live backing file change

Quoted 11 lines

 block.c                   |  80 ++++++++--------
 block/commit.c            |   9 +-
 block/stream.c            |  11 +--
 blockdev.c                | 238 ++++++++++++++++++++++++++++++++++++++++++----
 hmp.c                     |   1 +
 include/block/block.h     |   4 +-
 include/block/block_int.h |   3 +-
 qapi/block-core.json      | 145 +++++++++++++++++++++++++---
 qmp-commands.hx           | 181 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040    |  28 ++++--
 10 files changed, 602 insertions(+), 98 deletions(-)

This series side-steps lack of child op blockers by checking only the root node/drive.

Existing node-name commands like resize and snapshot-sync check for op blockers on the actual node. They do not take the same approach as this patch series.

We have a mess and I don't want to commit this series before we've figured out what to do about child op blockers.

Let's discuss this topic in a sub-thread and figure out what to do for QEMU 2.1. This is an important issue to solve before the release because we can't change QMP command semantics easily later.

My questions are:
a. How do we fix resize, snapshot-sync, etc? It seems like we need to
propagate child op blockers.

b. Is it a good idea to perform op blocker checks on the root node?
It's inconsistent with resize, snapshot-sync, etc. Permissions in BDS graphs with multiple root nodes (e.g. guest device and NBD run-time server) will be different depending on which root you specify.

From commit_run(), in block/commit.c (~line 89):

c. We're painting ourselves into a corner by using the root node for op
blocker checks. We'll have to apply the same op blockers to all nodes in a graph. There's no opportunity to apply different op blockers to a subset of the child nodes. I *think* this can be changed later without affecting the QMP API, so it's not a critical issue.

The answer seems to be that op blockers should be propagated to all child nodes and commands should test the node, not the drive/root node. That gives us the flexibility for per-node op blockers in the future and maintains compatibility with existing node-name users.

Stefan