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 <address@hidden>
> ---
>  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 <address@hidden>