[Qemu-devel] [PATCH v2] block: Validate node-name

[Qemu-devel] [PATCH v2] block: Validate node-name

[Kevin Wolf]

The device_name of a BlockDriverState is currently checked because it is
always used as a QemuOpts ID and qemu_opts_create() checks whether such
IDs are wellformed.

node-name is supposed to share the same namespace, but it isn't checked
currently. This patch adds explicit checks both for device_name and
node-name so that the same rules will still apply even if QemuOpts won't
be used any more at some point.

qemu-img used to use names with spaces in them, which isn't allowed any
more. Replace them with underscores.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
v2:
- Fix qemu-img to use valid names internally [Stefan]

 block.c               | 16 +++++++++++++---
 include/qemu/option.h |  1 +
 qemu-img.c            |  6 +++---
 util/qemu-option.c    |  4 ++--
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 6934018..ec92c12 100644
--- a/block.c
+++ b/block.c
@@ -335,12 +335,22 @@ void bdrv_register(BlockDriver *bdrv)
     QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
 }
 
+static bool bdrv_is_valid_name(const char *name)
+{
+    return qemu_opts_id_wellformed(name);
+}
+
 /* create a new block device (by default it is empty) */
 BlockDriverState *bdrv_new(const char *device_name, Error **errp)
 {
     BlockDriverState *bs;
     int i;
 
+    if (*device_name && !bdrv_is_valid_name(device_name)) {
+        error_setg(errp, "Invalid device name");
+        return NULL;
+    }
+
     if (bdrv_find(device_name)) {
         error_setg(errp, "Device with id '%s' already exists",
                    device_name);
@@ -863,9 +873,9 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
         return;
     }
 
-    /* empty string node name is invalid */
-    if (node_name[0] == '\0') {
-        error_setg(errp, "Empty node name");
+    /* Check for empty string or invalid characters */
+    if (!bdrv_is_valid_name(node_name)) {
+        error_setg(errp, "Invalid node name");
         return;
     }
 
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 59bea75..945347c 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -103,6 +103,7 @@ typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaq
 int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
                      int abort_on_failure);
 
+int qemu_opts_id_wellformed(const char *id);
 QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id);
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
                            int fail_if_exists, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index dbf0904..ea4bbae 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1011,14 +1011,14 @@ static int img_compare(int argc, char **argv)
         goto out3;
     }
 
-    bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
+    bs1 = bdrv_new_open("image_1", filename1, fmt1, flags, true, quiet);
     if (!bs1) {
         error_report("Can't open file %s", filename1);
         ret = 2;
         goto out3;
     }
 
-    bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
+    bs2 = bdrv_new_open("image_2", filename2, fmt2, flags, true, quiet);
     if (!bs2) {
         error_report("Can't open file %s", filename2);
         ret = 2;
@@ -1359,7 +1359,7 @@ static int img_convert(int argc, char **argv)
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
-        char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
+        char *id = bs_n > 1 ? g_strdup_printf("source_%d", bs_i)
                             : g_strdup("source");
         bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
                                  true, quiet);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6dc27ce..0cf9960 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -641,7 +641,7 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
     return NULL;
 }
 
-static int id_wellformed(const char *id)
+int qemu_opts_id_wellformed(const char *id)
 {
     int i;
 
@@ -662,7 +662,7 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
     QemuOpts *opts = NULL;
 
     if (id) {
-        if (!id_wellformed(id)) {
+        if (!qemu_opts_id_wellformed(id)) {
             error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
 #if 0 /* conversion from qerror_report() to error_set() broke this: */
             error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] block: Validate node-name

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]

On Thu, Sep 25, 2014 at 09:54:02AM +0200, Kevin Wolf wrote:
> The device_name of a BlockDriverState is currently checked because it is
> always used as a QemuOpts ID and qemu_opts_create() checks whether such
> IDs are wellformed.
> 
> node-name is supposed to share the same namespace, but it isn't checked
> currently. This patch adds explicit checks both for device_name and
> node-name so that the same rules will still apply even if QemuOpts won't
> be used any more at some point.
> 
> qemu-img used to use names with spaces in them, which isn't allowed any
> more. Replace them with underscores.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> v2:
> - Fix qemu-img to use valid names internally [Stefan]
> 
>  block.c               | 16 +++++++++++++---
>  include/qemu/option.h |  1 +
>  qemu-img.c            |  6 +++---
>  util/qemu-option.c    |  4 ++--
>  4 files changed, 19 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2] block: Validate node-name

[Kevin Wolf]

Am 25.09.2014 um 09:54 hat Kevin Wolf geschrieben:
> The device_name of a BlockDriverState is currently checked because it is
> always used as a QemuOpts ID and qemu_opts_create() checks whether such
> IDs are wellformed.
> 
> node-name is supposed to share the same namespace, but it isn't checked
> currently. This patch adds explicit checks both for device_name and
> node-name so that the same rules will still apply even if QemuOpts won't
> be used any more at some point.
> 
> qemu-img used to use names with spaces in them, which isn't allowed any
> more. Replace them with underscores.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Applied to the block branch.

Kevin