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

[Qemu-devel] [PATCH] 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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 16 +++++++++++++---
 include/qemu/option.h |  1 +
 util/qemu-option.c    |  4 ++--
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index e144fd5..bddf1a0 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);
@@ -903,9 +913,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/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] block: Validate node-name

[Benoît Canet]

> +int qemu_opts_id_wellformed(const char *id)

This return 0 and 1 as a bool.
Could we make the function return bool in the same series ?

I wonder what are the possible interferences between !strchr("-._", id[i])
and Jeff's node name auto naming series.

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

[Kevin Wolf]

Am 17.09.2014 um 13:49 hat Benoît Canet geschrieben:
> 
> 
> > +int qemu_opts_id_wellformed(const char *id)
> 
> This return 0 and 1 as a bool.
> Could we make the function return bool in the same series ?

I considered the change (as you probably saw, the new block.c function
returns a bool), but then thought it wasn't important enough.

In any case, that would be something for a separate patch. If you think
it's important, I can send one.

> I wonder what are the possible interferences between !strchr("-._", id[i])
> and Jeff's node name auto naming series.

We might need to update the code then, but it would actually be a good
reason why auto-naming wouldn't hurt if it uses characters that you
can't use manually.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.09.2014 um 13:49 hat Benoît Canet geschrieben:
>> 
>> 
>> > +int qemu_opts_id_wellformed(const char *id)
>> 
>> This return 0 and 1 as a bool.
>> Could we make the function return bool in the same series ?
>
> I considered the change (as you probably saw, the new block.c function
> returns a bool), but then thought it wasn't important enough.
>
> In any case, that would be something for a separate patch. If you think
> it's important, I can send one.
>
>> I wonder what are the possible interferences between !strchr("-._", id[i])
>> and Jeff's node name auto naming series.
>
> We might need to update the code then, but it would actually be a good
> reason why auto-naming wouldn't hurt if it uses characters that you
> can't use manually.

I'm afraid this is something we should ponder in a wider context, not
just BDS names.  Ties to other users of QemuOpts IDs, such as qdev, and
to how QOM lets users refer to objects.

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

[Benoît Canet]

The Wednesday 17 Sep 2014 à 13:31:06 (+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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 16 +++++++++++++---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    |  4 ++--
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e144fd5..bddf1a0 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);
> @@ -903,9 +913,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/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
> 

Reviewed-by: Benoit Canet <benoit.canet@nodalink.com>

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

[Stefan Hajnoczi]

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

On Wed, Sep 17, 2014 at 01:31:06PM +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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 16 +++++++++++++---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    |  4 ++--
>  3 files changed, 16 insertions(+), 5 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

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

[Stefan Hajnoczi]

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

On Wed, Sep 17, 2014 at 01:31:06PM +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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 16 +++++++++++++---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    |  4 ++--
>  3 files changed, 16 insertions(+), 5 deletions(-)

This patch breaks qemu-iotests 048 and 055.  Please fix and resend, I
have dropped it from the block branch:

048 3s ... - output mismatch (see 048.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/048.out	2014-09-16 16:37:56.617967668 +0100
+++ 048.out.bad	2014-09-19 14:19:44.428753385 +0100
@@ -9,25 +9,31 @@
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 4096/4096 bytes at offset 536576
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Images are identical.
-0
-0
+qemu-img: Invalid device name
+./048: line 39: 11542 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
+qemu-img: Invalid device name
+./048: line 39: 11547 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
 Image resized.
-Warning: Image size mismatch!
-Images are identical.
-0
-Strict mode: Image size mismatch!
-1
+qemu-img: Invalid device name
+./048: line 39: 11551 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
+qemu-img: Invalid device name
+./048: line 39: 11553 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
 === IO: pattern 67
 wrote 4096/4096 bytes at offset 1228800
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Content mismatch at offset 1228800!
-1
+qemu-img: Invalid device name
+./048: line 39: 11563 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
 === IO: pattern 123
 wrote 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Content mismatch at offset 0!
-1
+qemu-img: Invalid device name
+./048: line 39: 11573 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 === IO: pattern 100
 wrote 512/512 bytes at offset 0
@@ -35,6 +41,7 @@
 === IO: pattern 101
 wrote 512/512 bytes at offset 512
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Content mismatch at offset 512!
-1
+qemu-img: Invalid device name
+./048: line 39: 11596 Aborted                 (core dumped) $QEMU_IMG compare "$@" "$TEST_IMG" "${TEST_IMG2}"
+134
 Cleanup
055 9s ... [failed, exit status 1] - output mismatch (see 055.out.bad)
--- /home/stefanha/qemu/tests/qemu-iotests/055.out	2014-03-14 08:29:48.818442477 +0000
+++ 055.out.bad	2014-09-19 14:19:51.739791191 +0100
@@ -1,5 +1,23 @@
-..............
+qemu-img: Invalid device name
+qemu-img: Invalid device name
+.......F.....F
+======================================================================
+FAIL: test_pause (__main__.TestSingleDrive)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "055", line 90, in test_pause
+    'target image does not match source after backup')
+AssertionError: target image does not match source after backup
+
+======================================================================
+FAIL: test_pause (__main__.TestSingleTransaction)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "055", line 252, in test_pause
+    'target image does not match source after backup')
+AssertionError: target image does not match source after backup
+
 ----------------------------------------------------------------------
 Ran 14 tests
 
-OK
+FAILED (failures=2)
Failures: 048 055
Failed 2 of 2 tests

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