block: Validate node-name

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(-)

> +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.

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

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>

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.

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