[Qemu-devel] [PATCH v3 0/4]

[Qemu-devel] [PATCH v3 0/4]

[Jeff Cody]

Changes from v2:

    Patch 1:  Fixed prototype for id_generate() (thanks Alberto)
              Used *const instead of * const (thanks Eric, Markus)
              Updated function comment (thanks Markus)
              Made random in range 0-99 instead of 0-98 (thanks, Marksu)

    Patch 2: Cleaned up comments (thanks Markus)
             use else if instead of nested if (thanks Markus)
             assign node_name on same line as gen_node_name (thanks Markus)

    Patch 3,4: new - fix iotests (thanks Kevin)


Changes from RFC v1:

    Patch 1: Several typos / grammatical errors (thanks Eric, John)
             Make id_subsys_str[] const pointer to const strings (thanks Eric)
             Moved id_subsys_str[] out from  id_generate() (thanks John)
             Assert on null string for given id (thanks Eric)
             Zero-pad the 2-digit random # (thanks John)

    Patch 2: None

Born from the conversation on qemu-devel, this generation scheme uses the
format ultimately proposed by Kevin, after list discussion.

It attempts to keep the ID strings as small as possible, while fulfilling:
    
    1.) Guarantee no collisions with a user-specified ID
    2.) Identify the sub-system the ID belongs to
    3.) Guarantee of uniqueness
    4.) Spoiling predictibility, to avoid creating an assumption
        of object ordering and parsing (i.e., we don't want users to think
        they can guess the next ID based on prior behavior).

See patch 1 for the generation scheme details.

Jeff Cody (4):
  util - add automated ID generation utility
  block: auto-generated node-names
  block: add filter for generated node-names
  qemu-iotests: update tests for generated node-names

 block.c                          | 19 ++++++++++++-------
 include/qemu-common.h            |  8 ++++++++
 tests/qemu-iotests/041           |  4 ++--
 tests/qemu-iotests/051           |  3 ++-
 tests/qemu-iotests/051.out       |  2 +-
 tests/qemu-iotests/067           |  3 ++-
 tests/qemu-iotests/067.out       |  5 +++++
 tests/qemu-iotests/081           |  3 ++-
 tests/qemu-iotests/081.out       |  2 +-
 tests/qemu-iotests/common.filter |  5 +++++
 util/id.c                        | 37 +++++++++++++++++++++++++++++++++++++
 11 files changed, 77 insertions(+), 14 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[Jeff Cody]

Multiple sub-systems in QEMU may find it useful to generate IDs
for objects that a user may reference via QMP or HMP.  This patch
presents a standardized way to do it, so that automatic ID generation
follows the same rules.

This patch enforces the following rules when generating an ID:

1.) Guarantee no collisions with a user-specified ID
2.) Identify the sub-system the ID belongs to
3.) Guarantee of uniqueness
4.) Spoiling predictability, to avoid creating an assumption
    of object ordering and parsing (i.e., we don't want users to think
    they can guess the next ID based on prior behavior).

The scheme for this is as follows (no spaces):

                # subsys D RR
Reserved char --|    |   | |
Subsystem String ----|   | |
Unique number (64-bit) --| |
Two-digit random number ---|

For example, a generated node-name for the block sub-system may look
like this:

    #block076

The caller of id_generate() is responsible for freeing the generated
node name string with g_free().

Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 include/qemu-common.h |  8 ++++++++
 util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 0bd212b..2f74540 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
 #define STR_OR_NULL(str) ((str) ? (str) : "null")
 
 /* id.c */
+
+typedef enum IdSubSystems {
+    ID_QDEV,
+    ID_BLOCK,
+    ID_MAX      /* last element, used as array size */
+} IdSubSystems;
+
+char *id_generate(IdSubSystems id);
 bool id_wellformed(const char *id);
 
 /* path.c */
diff --git a/util/id.c b/util/id.c
index 09b22fb..bcc64d8 100644
--- a/util/id.c
+++ b/util/id.c
@@ -26,3 +26,40 @@ bool id_wellformed(const char *id)
     }
     return true;
 }
+
+#define ID_SPECIAL_CHAR '#'
+
+static const char *const id_subsys_str[] = {
+    [ID_QDEV]  = "qdev",
+    [ID_BLOCK] = "block",
+};
+
+/*
+ *  Generates an ID of the form PREFIX SUBSYSTEM NUMBER
+ *  where:
+ *
+ *  - PREFIX is the reserved character '#'
+ *  - SUBSYSTEM identifies the subsystem creating the ID
+ *  - NUMBER is a decimal number unique within SUBSYSTEM.
+ *
+ *    Example: "#block146"
+ *
+ * Note that these IDs do not satisfy id_wellformed().
+ *
+ * The caller is responsible for freeing the returned string with g_free()
+ */
+char *id_generate(IdSubSystems id)
+{
+    static uint64_t id_counters[ID_MAX];
+    uint32_t rnd;
+
+    assert(id < ID_MAX);
+    assert(id_subsys_str[id]);
+
+    rnd = g_random_int_range(0, 100);
+
+    return g_strdup_printf("%c%s%" PRIu64 "%02" PRId32, ID_SPECIAL_CHAR,
+                                                        id_subsys_str[id],
+                                                        id_counters[id]++,
+                                                        rnd);
+}
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/4] block: auto-generated node-names

[Jeff Cody]

If a node-name is not specified, automatically generate the node-name.

Generated node-names will use the "block" sub-system identifier.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 1f90b47..5947704 100644
--- a/block.c
+++ b/block.c
@@ -763,12 +763,15 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
                                   const char *node_name,
                                   Error **errp)
 {
+    char *gen_node_name = NULL;
+
     if (!node_name) {
-        return;
-    }
-
-    /* Check for empty string or invalid characters */
-    if (!id_wellformed(node_name)) {
+        node_name = gen_node_name = id_generate(ID_BLOCK);
+    } else if (!id_wellformed(node_name)) {
+        /*
+         * Check for empty string or invalid characters, but not if it is
+         * generated (generated names use characters not available to the user)
+         */
         error_setg(errp, "Invalid node name");
         return;
     }
@@ -777,18 +780,20 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
     if (blk_by_name(node_name)) {
         error_setg(errp, "node-name=%s is conflicting with a device id",
                    node_name);
-        return;
+        goto out;
     }
 
     /* takes care of avoiding duplicates node names */
     if (bdrv_find_node(node_name)) {
         error_setg(errp, "Duplicate node name");
-        return;
+        goto out;
     }
 
     /* copy node name into the bs and insert it into the graph list */
     pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
     QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
+out:
+    g_free(gen_node_name);
 }
 
 static QemuOptsList bdrv_runtime_opts = {
-- 
1.9.3

[Qemu-devel] [PATCH v3 3/4] block: add filter for generated node-names

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/common.filter | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index d6d05de..cfdb633 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -128,6 +128,11 @@ _filter_date()
         -e 's/[A-Z][a-z][a-z] [A-z][a-z][a-z]  *[0-9][0-9]* [0-9][0-9]:[0-9][0-9]:[0-9][0-9] [0-9][0-9][0-9][0-9]$/DATE/'
 }
 
+_filter_generated_node_ids()
+{
+     sed -re 's/\#block[0-9]{3,}/NODE_NAME/'
+}
+
 # replace occurrences of the actual TEST_DIR value with TEST_DIR
 _filter_testdir()
 {
-- 
1.9.3

[Qemu-devel] [PATCH v3 4/4] qemu-iotests: update tests for generated node-names

[Jeff Cody]

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/041     | 4 ++--
 tests/qemu-iotests/051     | 3 ++-
 tests/qemu-iotests/051.out | 2 +-
 tests/qemu-iotests/067     | 3 ++-
 tests/qemu-iotests/067.out | 5 +++++
 tests/qemu-iotests/081     | 3 ++-
 tests/qemu-iotests/081.out | 2 +-
 7 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 59c1a76..05b5962 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -780,7 +780,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         # here we check that the last registered quorum file has not been
         # swapped out and unref
         result = self.vm.qmp('query-named-block-nodes')
-        self.assert_qmp(result, 'return[0]/file', quorum_img3)
+        self.assert_qmp(result, 'return[1]/file', quorum_img3)
         self.vm.shutdown()
 
     def test_cancel_after_ready(self):
@@ -799,7 +799,7 @@ class TestRepairQuorum(iotests.QMPTestCase):
         result = self.vm.qmp('query-named-block-nodes')
         # here we check that the last registered quorum file has not been
         # swapped out and unref
-        self.assert_qmp(result, 'return[0]/file', quorum_img3)
+        self.assert_qmp(result, 'return[1]/file', quorum_img3)
         self.vm.shutdown()
         self.assertTrue(iotests.compare_images(quorum_img2, quorum_repair_img),
                         'target image does not match source after mirroring')
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 4a8055b..17dbf04 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -108,7 +108,8 @@ echo
 echo === Overriding backing file ===
 echo
 
-echo "info block" | run_qemu -drive file="$TEST_IMG",driver=qcow2,backing.file.filename="$TEST_IMG.orig" -nodefaults
+echo "info block" | run_qemu -drive file="$TEST_IMG",driver=qcow2,backing.file.filename="$TEST_IMG.orig" -nodefaults\
+                  | _filter_generated_node_ids
 
 # Drivers that don't support backing files
 run_qemu -drive file="$TEST_IMG",driver=raw,backing.file.filename="$TEST_IMG.orig"
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 0429be2..7765aa0 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -59,7 +59,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive file=TEST_DIR/t.qcow2,driver=qcow2,backing.file.filename=TEST_DIR/t.qcow2.orig -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
-ide0-hd0: TEST_DIR/t.qcow2 (qcow2)
+ide0-hd0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
     Cache mode:       writeback
     Backing file:     TEST_DIR/t.qcow2.orig (chain depth: 1)
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
index 3e9a053..3788534 100755
--- a/tests/qemu-iotests/067
+++ b/tests/qemu-iotests/067
@@ -48,7 +48,8 @@ function do_run_qemu()
 function run_qemu()
 {
     do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu \
-                          | sed -e 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g'
+                          | sed -e 's/\("actual-size":\s*\)[0-9]\+/\1SIZE/g' \
+                          | _filter_generated_node_ids
 }
 
 size=128M
diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out
index 5fbc881..27ad56f 100644
--- a/tests/qemu-iotests/067.out
+++ b/tests/qemu-iotests/067.out
@@ -40,6 +40,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk -device virti
                 },
                 "iops_wr": 0,
                 "ro": false,
+                "node-name": "NODE_NAME",
                 "backing_file_depth": 0,
                 "drv": "qcow2",
                 "iops": 0,
@@ -151,6 +152,7 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk
                 },
                 "iops_wr": 0,
                 "ro": false,
+                "node-name": "NODE_NAME",
                 "backing_file_depth": 0,
                 "drv": "qcow2",
                 "iops": 0,
@@ -270,6 +272,7 @@ Testing:
                 },
                 "iops_wr": 0,
                 "ro": false,
+                "node-name": "NODE_NAME",
                 "backing_file_depth": 0,
                 "drv": "qcow2",
                 "iops": 0,
@@ -390,6 +393,7 @@ Testing:
                 },
                 "iops_wr": 0,
                 "ro": false,
+                "node-name": "NODE_NAME",
                 "backing_file_depth": 0,
                 "drv": "qcow2",
                 "iops": 0,
@@ -480,6 +484,7 @@ Testing:
                 },
                 "iops_wr": 0,
                 "ro": false,
+                "node-name": "NODE_NAME",
                 "backing_file_depth": 0,
                 "drv": "qcow2",
                 "iops": 0,
diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index d9b042c..51873ff 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -53,7 +53,8 @@ function do_run_qemu()
 
 function run_qemu()
 {
-    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp | _filter_qemu_io
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
+                          | _filter_qemu_io | _filter_generated_node_ids
 }
 
 test_quorum=$($QEMU_IMG --help|grep quorum)
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 692c4a4..044793d 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -30,7 +30,7 @@ Testing: -drive file=TEST_DIR/2.IMGFMT,format=IMGFMT,if=none,id=drive2
 QMP_VERSION
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "", "sectors-count": 20480, "sector-num": 0}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "NODE_NAME", "sectors-count": 20480, "sector-num": 0}}
 read 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 {"return": ""}
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> Multiple sub-systems in QEMU may find it useful to generate IDs
> for objects that a user may reference via QMP or HMP.  This patch
> presents a standardized way to do it, so that automatic ID generation
> follows the same rules.
>
> This patch enforces the following rules when generating an ID:
>
> 1.) Guarantee no collisions with a user-specified ID
> 2.) Identify the sub-system the ID belongs to
> 3.) Guarantee of uniqueness
> 4.) Spoiling predictability, to avoid creating an assumption
>     of object ordering and parsing (i.e., we don't want users to think
>     they can guess the next ID based on prior behavior).
>
> The scheme for this is as follows (no spaces):
>
>                 # subsys D RR
> Reserved char --|    |   | |
> Subsystem String ----|   | |
> Unique number (64-bit) --| |
> Two-digit random number ---|
>
> For example, a generated node-name for the block sub-system may look
> like this:
>
>     #block076
>
> The caller of id_generate() is responsible for freeing the generated
> node name string with g_free().
>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  include/qemu-common.h |  8 ++++++++
>  util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 0bd212b..2f74540 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
>  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>  
>  /* id.c */
> +
> +typedef enum IdSubSystems {
> +    ID_QDEV,

ID_QDEV is not used in this series.  Do you intend to use it in a
followup-series?  Can we reasonably expect that series will be accepted?

You could sidestep these questions by making id_generate() take a string
argument ;)

> +    ID_BLOCK,
> +    ID_MAX      /* last element, used as array size */
> +} IdSubSystems;
> +
> +char *id_generate(IdSubSystems id);
>  bool id_wellformed(const char *id);
>  
>  /* path.c */
[...]

Re: [Qemu-devel] [PATCH v3 2/4] block: auto-generated node-names

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> If a node-name is not specified, automatically generate the node-name.
>
> Generated node-names will use the "block" sub-system identifier.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1f90b47..5947704 100644
> --- a/block.c
> +++ b/block.c
> @@ -763,12 +763,15 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
>                                    const char *node_name,
>                                    Error **errp)
>  {
> +    char *gen_node_name = NULL;
> +
>      if (!node_name) {
> -        return;
> -    }
> -
> -    /* Check for empty string or invalid characters */
> -    if (!id_wellformed(node_name)) {
> +        node_name = gen_node_name = id_generate(ID_BLOCK);
> +    } else if (!id_wellformed(node_name)) {
> +        /*
> +         * Check for empty string or invalid characters, but not if it is
> +         * generated (generated names use characters not available to the user)
> +         */
>          error_setg(errp, "Invalid node name");
>          return;
>      }

I'd drop the comment, as the code is obvious enough now.  Could be done
on commit.

> @@ -777,18 +780,20 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
>      if (blk_by_name(node_name)) {
>          error_setg(errp, "node-name=%s is conflicting with a device id",
>                     node_name);
> -        return;
> +        goto out;
>      }
>  
>      /* takes care of avoiding duplicates node names */
>      if (bdrv_find_node(node_name)) {
>          error_setg(errp, "Duplicate node name");
> -        return;
> +        goto out;
>      }
>  
>      /* copy node name into the bs and insert it into the graph list */
>      pstrcpy(bs->node_name, sizeof(bs->node_name), node_name);
>      QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
> +out:
> +    g_free(gen_node_name);
>  }
>  
>  static QemuOptsList bdrv_runtime_opts = {

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: update tests for generated node-names

[Markus Armbruster]

Shouldn't PATCH 3+4 be squashed into PATCH 2 to keep bisection working?

Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[Jeff Cody]

On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
> > Multiple sub-systems in QEMU may find it useful to generate IDs
> > for objects that a user may reference via QMP or HMP.  This patch
> > presents a standardized way to do it, so that automatic ID generation
> > follows the same rules.
> >
> > This patch enforces the following rules when generating an ID:
> >
> > 1.) Guarantee no collisions with a user-specified ID
> > 2.) Identify the sub-system the ID belongs to
> > 3.) Guarantee of uniqueness
> > 4.) Spoiling predictability, to avoid creating an assumption
> >     of object ordering and parsing (i.e., we don't want users to think
> >     they can guess the next ID based on prior behavior).
> >
> > The scheme for this is as follows (no spaces):
> >
> >                 # subsys D RR
> > Reserved char --|    |   | |
> > Subsystem String ----|   | |
> > Unique number (64-bit) --| |
> > Two-digit random number ---|
> >
> > For example, a generated node-name for the block sub-system may look
> > like this:
> >
> >     #block076
> >
> > The caller of id_generate() is responsible for freeing the generated
> > node name string with g_free().
> >
> > Reviewed-by: John Snow <jsnow@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  include/qemu-common.h |  8 ++++++++
> >  util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff --git a/include/qemu-common.h b/include/qemu-common.h
> > index 0bd212b..2f74540 100644
> > --- a/include/qemu-common.h
> > +++ b/include/qemu-common.h
> > @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
> >  #define STR_OR_NULL(str) ((str) ? (str) : "null")
> >  
> >  /* id.c */
> > +
> > +typedef enum IdSubSystems {
> > +    ID_QDEV,
> 
> ID_QDEV is not used in this series.  Do you intend to use it in a
> followup-series?  Can we reasonably expect that series will be accepted?
> 

John Arbuckle has a patch on list that uses it.  I haven't reviewed
it, however - but I guess it depends ultimately on whether qdev will
allow autogeneration for its IDs or not.

> You could sidestep these questions by making id_generate() take a string
> argument ;)
> 

I'd rather avoid having each system specifying a string inline in
their code.  It is cleaner to have the strings defined in a central
location, I think (not to mention, easier to reference).

> > +    ID_BLOCK,
> > +    ID_MAX      /* last element, used as array size */
> > +} IdSubSystems;
> > +
> > +char *id_generate(IdSubSystems id);
> >  bool id_wellformed(const char *id);
> >  
> >  /* path.c */
> [...]

Re: [Qemu-devel] [PATCH v3 4/4] qemu-iotests: update tests for generated node-names

[Jeff Cody]

On Tue, Oct 13, 2015 at 09:41:13AM +0200, Markus Armbruster wrote:
> Shouldn't PATCH 3+4 be squashed into PATCH 2 to keep bisection working?

Yes, they should!

Re: [Qemu-devel] [PATCH v3 0/4]

[Kevin Wolf]

Am 13.10.2015 um 01:36 hat Jeff Cody geschrieben:
> Born from the conversation on qemu-devel, this generation scheme uses the
> format ultimately proposed by Kevin, after list discussion.
> 
> It attempts to keep the ID strings as small as possible, while fulfilling:
>     
>     1.) Guarantee no collisions with a user-specified ID
>     2.) Identify the sub-system the ID belongs to
>     3.) Guarantee of uniqueness
>     4.) Spoiling predictibility, to avoid creating an assumption
>         of object ordering and parsing (i.e., we don't want users to think
>         they can guess the next ID based on prior behavior).
> 
> See patch 1 for the generation scheme details.

Thanks, squashed patches 2-4 and applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[Markus Armbruster]

Jeff Cody <jcody@redhat.com> writes:

> On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote:
>> Jeff Cody <jcody@redhat.com> writes:
>> 
>> > Multiple sub-systems in QEMU may find it useful to generate IDs
>> > for objects that a user may reference via QMP or HMP.  This patch
>> > presents a standardized way to do it, so that automatic ID generation
>> > follows the same rules.
>> >
>> > This patch enforces the following rules when generating an ID:
>> >
>> > 1.) Guarantee no collisions with a user-specified ID
>> > 2.) Identify the sub-system the ID belongs to
>> > 3.) Guarantee of uniqueness
>> > 4.) Spoiling predictability, to avoid creating an assumption
>> >     of object ordering and parsing (i.e., we don't want users to think
>> >     they can guess the next ID based on prior behavior).
>> >
>> > The scheme for this is as follows (no spaces):
>> >
>> >                 # subsys D RR
>> > Reserved char --|    |   | |
>> > Subsystem String ----|   | |
>> > Unique number (64-bit) --| |
>> > Two-digit random number ---|
>> >
>> > For example, a generated node-name for the block sub-system may look
>> > like this:
>> >
>> >     #block076
>> >
>> > The caller of id_generate() is responsible for freeing the generated
>> > node name string with g_free().
>> >
>> > Reviewed-by: John Snow <jsnow@redhat.com>
>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> > Reviewed-by: Alberto Garcia <berto@igalia.com>
>> > Signed-off-by: Jeff Cody <jcody@redhat.com>
>> > ---
>> >  include/qemu-common.h |  8 ++++++++
>> >  util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 45 insertions(+)
>> >
>> > diff --git a/include/qemu-common.h b/include/qemu-common.h
>> > index 0bd212b..2f74540 100644
>> > --- a/include/qemu-common.h
>> > +++ b/include/qemu-common.h
>> > @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
>> >  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>> >  
>> >  /* id.c */
>> > +
>> > +typedef enum IdSubSystems {
>> > +    ID_QDEV,
>> 
>> ID_QDEV is not used in this series.  Do you intend to use it in a
>> followup-series?  Can we reasonably expect that series will be accepted?
>> 
>
> John Arbuckle has a patch on list that uses it.  I haven't reviewed
> it, however - but I guess it depends ultimately on whether qdev will
> allow autogeneration for its IDs or not.

Then that patch should add ID_QDEV.

>> You could sidestep these questions by making id_generate() take a string
>> argument ;)
>> 
>
> I'd rather avoid having each system specifying a string inline in
> their code.  It is cleaner to have the strings defined in a central
> location, I think (not to mention, easier to reference).

Covered by your artistic license :)

>> > +    ID_BLOCK,
>> > +    ID_MAX      /* last element, used as array size */
>> > +} IdSubSystems;
>> > +
>> > +char *id_generate(IdSubSystems id);
>> >  bool id_wellformed(const char *id);
>> >  
>> >  /* path.c */
>> [...]

Re: [Qemu-devel] [PATCH v3 0/4]

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.10.2015 um 01:36 hat Jeff Cody geschrieben:
>> Born from the conversation on qemu-devel, this generation scheme uses the
>> format ultimately proposed by Kevin, after list discussion.
>> 
>> It attempts to keep the ID strings as small as possible, while fulfilling:
>>     
>>     1.) Guarantee no collisions with a user-specified ID
>>     2.) Identify the sub-system the ID belongs to
>>     3.) Guarantee of uniqueness
>>     4.) Spoiling predictibility, to avoid creating an assumption
>>         of object ordering and parsing (i.e., we don't want users to think
>>         they can guess the next ID based on prior behavior).
>> 
>> See patch 1 for the generation scheme details.
>
> Thanks, squashed patches 2-4 and applied to the block branch.

If you drop ID_QDEV, you can add my R-by.

Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[John Snow]

On 10/13/2015 11:26 AM, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> writes:
> 
>> On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote:
>>> Jeff Cody <jcody@redhat.com> writes:
>>>
>>>> Multiple sub-systems in QEMU may find it useful to generate IDs
>>>> for objects that a user may reference via QMP or HMP.  This patch
>>>> presents a standardized way to do it, so that automatic ID generation
>>>> follows the same rules.
>>>>
>>>> This patch enforces the following rules when generating an ID:
>>>>
>>>> 1.) Guarantee no collisions with a user-specified ID
>>>> 2.) Identify the sub-system the ID belongs to
>>>> 3.) Guarantee of uniqueness
>>>> 4.) Spoiling predictability, to avoid creating an assumption
>>>>     of object ordering and parsing (i.e., we don't want users to think
>>>>     they can guess the next ID based on prior behavior).
>>>>
>>>> The scheme for this is as follows (no spaces):
>>>>
>>>>                 # subsys D RR
>>>> Reserved char --|    |   | |
>>>> Subsystem String ----|   | |
>>>> Unique number (64-bit) --| |
>>>> Two-digit random number ---|
>>>>
>>>> For example, a generated node-name for the block sub-system may look
>>>> like this:
>>>>
>>>>     #block076
>>>>
>>>> The caller of id_generate() is responsible for freeing the generated
>>>> node name string with g_free().
>>>>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>>> ---
>>>>  include/qemu-common.h |  8 ++++++++
>>>>  util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>>>> index 0bd212b..2f74540 100644
>>>> --- a/include/qemu-common.h
>>>> +++ b/include/qemu-common.h
>>>> @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
>>>>  #define STR_OR_NULL(str) ((str) ? (str) : "null")
>>>>  
>>>>  /* id.c */
>>>> +
>>>> +typedef enum IdSubSystems {
>>>> +    ID_QDEV,
>>>
>>> ID_QDEV is not used in this series.  Do you intend to use it in a
>>> followup-series?  Can we reasonably expect that series will be accepted?
>>>
>>
>> John Arbuckle has a patch on list that uses it.  I haven't reviewed
>> it, however - but I guess it depends ultimately on whether qdev will
>> allow autogeneration for its IDs or not.
> 
> Then that patch should add ID_QDEV.
> 
>>> You could sidestep these questions by making id_generate() take a string
>>> argument ;)
>>>
>>
>> I'd rather avoid having each system specifying a string inline in
>> their code.  It is cleaner to have the strings defined in a central
>> location, I think (not to mention, easier to reference).
> 
> Covered by your artistic license :)
> 

I think our engineering license would have us do it the way Jeff already
is -- If we want a central unique ID generator utility, it would be best
to disallow different areas of code from specifying their own IDs ...
it's bound to lead to collisions and heartbreak someday.

Easiest to just keep an enum and if you decide you need to use this
facility, add your name to the Registry Of Accepted Subcomponents and
keep moving.

>>>> +    ID_BLOCK,
>>>> +    ID_MAX      /* last element, used as array size */
>>>> +} IdSubSystems;
>>>> +
>>>> +char *id_generate(IdSubSystems id);
>>>>  bool id_wellformed(const char *id);
>>>>  
>>>>  /* path.c */
>>> [...]

Re: [Qemu-devel] [PATCH v3 1/4] util - add automated ID generation utility

[Programmingkid]

On Oct 13, 2015, at 11:26 AM, Markus Armbruster wrote:

> Jeff Cody <jcody@redhat.com> writes:
> 
>> On Tue, Oct 13, 2015 at 09:37:29AM +0200, Markus Armbruster wrote:
>>> Jeff Cody <jcody@redhat.com> writes:
>>> 
>>>> Multiple sub-systems in QEMU may find it useful to generate IDs
>>>> for objects that a user may reference via QMP or HMP.  This patch
>>>> presents a standardized way to do it, so that automatic ID generation
>>>> follows the same rules.
>>>> 
>>>> This patch enforces the following rules when generating an ID:
>>>> 
>>>> 1.) Guarantee no collisions with a user-specified ID
>>>> 2.) Identify the sub-system the ID belongs to
>>>> 3.) Guarantee of uniqueness
>>>> 4.) Spoiling predictability, to avoid creating an assumption
>>>>    of object ordering and parsing (i.e., we don't want users to think
>>>>    they can guess the next ID based on prior behavior).
>>>> 
>>>> The scheme for this is as follows (no spaces):
>>>> 
>>>>                # subsys D RR
>>>> Reserved char --|    |   | |
>>>> Subsystem String ----|   | |
>>>> Unique number (64-bit) --| |
>>>> Two-digit random number ---|
>>>> 
>>>> For example, a generated node-name for the block sub-system may look
>>>> like this:
>>>> 
>>>>    #block076
>>>> 
>>>> The caller of id_generate() is responsible for freeing the generated
>>>> node name string with g_free().
>>>> 
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>>>> ---
>>>> include/qemu-common.h |  8 ++++++++
>>>> util/id.c             | 37 +++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 45 insertions(+)
>>>> 
>>>> diff --git a/include/qemu-common.h b/include/qemu-common.h
>>>> index 0bd212b..2f74540 100644
>>>> --- a/include/qemu-common.h
>>>> +++ b/include/qemu-common.h
>>>> @@ -246,6 +246,14 @@ int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
>>>> #define STR_OR_NULL(str) ((str) ? (str) : "null")
>>>> 
>>>> /* id.c */
>>>> +
>>>> +typedef enum IdSubSystems {
>>>> +    ID_QDEV,
>>> 
>>> ID_QDEV is not used in this series.  Do you intend to use it in a
>>> followup-series?  Can we reasonably expect that series will be accepted?
>>> 
>> 
>> John Arbuckle has a patch on list that uses it.  I haven't reviewed
>> it, however - but I guess it depends ultimately on whether qdev will
>> allow autogeneration for its IDs or not.
> 
> Then that patch should add ID_QDEV.
> 
>>> You could sidestep these questions by making id_generate() take a string
>>> argument ;)
>>> 
>> 
>> I'd rather avoid having each system specifying a string inline in
>> their code.  It is cleaner to have the strings defined in a central
>> location, I think (not to mention, easier to reference).

I can see the benefit of using a string. The id_generate() function
could use va_args like printf() uses to allow almost any kind of 
string argument. An empty string argument could mean to default to
ID_MAX. But I also think using an enumeration is good enough, so 
either way is good.