Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 2/5] spice-qemu-char: fix parameter checks in qemu_chr_parse_* functions

["Alex Bennée" <alex.bennee@linaro.org>]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> For functions qemu_chr_parse_spice_vmc and qemu_chr_parse_spice_port,
> we should also check if parameter name is empty, and it will help finding
> the wrong configure, such as
> 'qemu-system-x86_64 -chardev spiceport(or spiceport),id=id,name='
>
> After check the parameter in parse function, we can remove the
> superfluous parameter checks in qemu_chr_open_spice_*
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  spice-qemu-char.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 8106e06..d3c1f5c 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -290,11 +290,6 @@ CharDriverState *qemu_chr_open_spice_vmc(const char *type)
>  {
>      const char **psubtype = spice_server_char_device_recognized_subtypes();
>  
> -    if (type == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        print_allowed_subtypes();
> -        return NULL;
> -    }

As with patch 1 I'd at least assert the contract. But this also drops a
helpful info dump on a missing parameter.

>      for (; *psubtype != NULL; ++psubtype) {
>          if (strcmp(type, *psubtype) == 0) {
>              break;
> @@ -315,11 +310,6 @@ CharDriverState *qemu_chr_open_spice_port(const char *name)
>      CharDriverState *chr;
>      SpiceCharDriver *s;
>  
> -    if (name == NULL) {
> -        fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> -        return NULL;
> -    }
> -
>      chr = chr_open("port", spice_port_set_fe_open);
>      s = chr->opaque;
>      s->sin.portname = g_strdup(name);
> @@ -345,7 +335,7 @@ static void qemu_chr_parse_spice_vmc(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice channel: no name given");
>          return;
>      }
> @@ -358,7 +348,7 @@ static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
>  {
>      const char *name = qemu_opt_get(opts, "name");
>  
> -    if (name == NULL) {
> +    if (name == NULL || !name[0]) {
>          error_setg(errp, "chardev: spice port: no name given");
>          return;
>      }

Same comments as for 1/5

-- 
Alex Bennée