Re: [Qemu-devel] [PATCH v3 10/13] hmp: add hmp analogue for qmp-chardev-change

[Anton Nefedov]

On 06/01/2017 10:12 AM, Marc-André Lureau wrote:
> Hi
>
> On Tue, May 30, 2017 at 6:08 PM Anton Nefedov 
> <address@hidden <mailto:address@hidden>> wrote:
>
>     Signed-off-by: Anton Nefedov <address@hidden
>     <mailto:address@hidden>>
>     Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden
>     <mailto:address@hidden>>
>     CC: Dr. David Alan Gilbert <address@hidden
>     <mailto:address@hidden>>
>     ---
>       chardev/char.c        |  2 +-
>       hmp-commands.hx       | 16 ++++++++++++++++
>       hmp.c                 | 34 ++++++++++++++++++++++++++++++++++
>       hmp.h                 |  1 +
>       include/sysemu/char.h | 12 ++++++++++++
>       5 files changed, 64 insertions(+), 1 deletion(-)
>
>     diff --git a/chardev/char.c b/chardev/char.c
>     index 2d6e204..1e2a2dd 100644
>     --- a/chardev/char.c
>     +++ b/chardev/char.c
>     @@ -888,7 +888,7 @@ help_string_append(const char *name, void *opaque)
>           g_string_append_printf(str, "\n%s", name);
>       }
>
>     -static ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error
>     **errp)
>     +ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts, Error **errp)
>       {
>           Error *local_err = NULL;
>           const ChardevClass *cc;
>     diff --git a/hmp-commands.hx b/hmp-commands.hx
>     index baeac47..19bfddd 100644
>     --- a/hmp-commands.hx
>     +++ b/hmp-commands.hx
>     @@ -1727,6 +1727,22 @@ chardev_add accepts the same parameters as
>     the -chardev command line switch.
>       ETEXI
>
>           {
>     +        .name       = "chardev-change",
>     +        .args_type  = "id:s,args:s",
>     +        .params     = "id args",
>     +        .help       = "change chardev",
>     +        .cmd        = hmp_chardev_change,
>     +    },
>     +
>     +STEXI
>     address@hidden chardev-change args
>     address@hidden chardev-change
>     +chardev_change accepts existing chardev @var{id} and then the same
>     arguments
>
>
> chardev_change vs chardev-change. Apparently, there is this difference 
> for chardev-add too

Done.

>     +as the -chardev command line switch (except for "id").
>     +
>     +ETEXI
>     +
>     +    {
>               .name       = "chardev-remove",
>               .args_type  = "id:s",
>               .params     = "id",
>     diff --git a/hmp.c b/hmp.c
>     index 20f5dab..7660495 100644
>     --- a/hmp.c
>     +++ b/hmp.c
>     @@ -2224,6 +2224,40 @@ void hmp_chardev_add(Monitor *mon, const
>     QDict *qdict)
>           hmp_handle_error(mon, &err);
>       }
>
>     +void hmp_chardev_change(Monitor *mon, const QDict *qdict)
>     +{
>     +    const char *args = qdict_get_str(qdict, "args");
>     +    const char *id;
>     +    Error *err = NULL;
>     +    ChardevBackend *backend = NULL;
>     +    ChardevReturn *ret = NULL;
>     +    QemuOpts *opts =
>     qemu_opts_parse_noisily(qemu_find_opts("chardev"), args,
>     +                                             true);
>     +    if (!opts) {
>     +        error_setg(&err, "Parsing chardev args failed");
>     +        goto end;
>     +    }
>     +
>     +    id = qdict_get_str(qdict, "id");
>     +    if (qemu_opts_id(opts)) {
>     +        error_setg(&err, "Unexpected 'id' parameter");
>
>
> enlighten me, how is this triggered?

correct variant:

  chardev-change char2 pipe,path=/tmp/cent.fifo

vs. (without this check)

  (qemu) chardev-change char2 pipe,id=char2,path=/tmp/cent.fifo
  chardev-change char2 pipe,id=char2,path=/tmp/cent.fifo
  Duplicate ID 'char2' for chardev
  Parsing chardev args failed

or

  (qemu) chardev-change char2 pipe,id=newchar3,path=/tmp/cent.fifo

the command would succeed but the user might expect the id to change,
but it wouldn't

>     +        goto end;
>     +    }
>     +
>     +    backend = qemu_chr_parse_opts(opts, &err);
>     +    if (!backend) {
>     +        goto end;
>     +    }
>     +
>     +    ret = qmp_chardev_change(id, backend, &err);
>     +
>     +end:
>     +    qapi_free_ChardevReturn(ret);
>     +    qapi_free_ChardevBackend(backend);
>     +    qemu_opts_del(opts);
>     +    hmp_handle_error(mon, &err);
>     +}
>     +
>       void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
>       {
>           Error *local_err = NULL;
>     diff --git a/hmp.h b/hmp.h
>     index d8b94ce..23e035c 100644
>     --- a/hmp.h
>     +++ b/hmp.h
>     @@ -102,6 +102,7 @@ void hmp_nbd_server_start(Monitor *mon, const
>     QDict *qdict);
>       void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
>       void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>       void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>     +void hmp_chardev_change(Monitor *mon, const QDict *qdict);
>       void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
>       void hmp_qemu_io(Monitor *mon, const QDict *qdict);
>       void hmp_cpu_add(Monitor *mon, const QDict *qdict);
>     diff --git a/include/sysemu/char.h b/include/sysemu/char.h
>     index 342f531..18fcd26 100644
>     --- a/include/sysemu/char.h
>     +++ b/include/sysemu/char.h
>     @@ -131,6 +131,18 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>       void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
>
>       /**
>     + * @qemu_chr_parse_opts:
>     + *
>     + * Parse the options to the ChardevBackend struct.
>     + *
>     + * @opts
>     + *
>
>
> You may remove that line
>
>     + * Returns: a new backend
>
>
> or NULL on error

Done.

>     + */
>     +ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>     +                                    Error **errp);
>     +
>     +/**
>        * @qemu_chr_new:
>        *
>        * Create a new character backend from a URI.
>     --
>     2.7.4
>
>
> --
> Marc-André Lureau

/Anton