[v2] chardev: add hotplug support.

This patch adds chardev_add and chardev_del monitor commands.

chardev_del is pretty straight forward, it just takes an id argument and
zaps the chardev specified.

chardev_add is more tricky as there are tons of arguments for the
different backends.  The hmp version limited to the most common use
cases, especially when it comes to sockets:  You can only specify port
(tcp) or path (unix) and qemu will create a listening socket.  For
example this ...

   (qemu) chardev_add foo socket 42

... will do the same as ...

   -chardev socket,id=foo,port=42,server,nowait

on the qemu command line.

The qmp version has full support for everything the -chardev command
line switch can handle.  The implementation is pretty straight
forward: It just puts all arguments it got into a QemuOpts, then goes
call qemu_chr_new_from_opts().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hmp-commands.hx  |   32 ++++++++++++++++++++++++++++
 hmp.c            |   31 +++++++++++++++++++++++++++
 hmp.h            |    2 +
 qapi-schema.json |   39 ++++++++++++++++++++++++++++++++++
 qemu-char.c      |   50 +++++++++++++++++++++++++++++++++++++++++++-
 qemu-char.h      |    2 +
 qmp-commands.hx  |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 216 insertions(+), 1 deletions(-)

On 10/12/12 14:39, Gerd Hoffmann wrote:
> This patch adds chardev_add and chardev_del monitor commands.
> 
> chardev_del is pretty straight forward, it just takes an id argument and
> zaps the chardev specified.
> 
> chardev_add is more tricky as there are tons of arguments for the

Oops.  Ignore the commit message for now, it obviously needs to be
updated to reflect the changes, will fix for v3, awaiting code reviews
meanwhile.

cheers,
  Gerd

Il 12/10/2012 14:39, Gerd Hoffmann ha scritto:
> +    chr = qemu_chr_new_from_opts(opts, NULL);
> +    if (chr == NULL) {
> +        qemu_opts_del(opts);
> +        error_setg(&err, "Creating chardev failed\n");
> +        goto exit_err;
> +    }
> +    return 0;
> +

Since you have to do a v3, please make this part (which is common
between HMP and QMP) a separate function.

Paolo

Il 12/10/2012 18:54, Paolo Bonzini ha scritto:
> Il 12/10/2012 14:39, Gerd Hoffmann ha scritto:
>> +    chr = qemu_chr_new_from_opts(opts, NULL);
>> +    if (chr == NULL) {
>> +        qemu_opts_del(opts);
>> +        error_setg(&err, "Creating chardev failed\n");
>> +        goto exit_err;
>> +    }
>> +    return 0;
>> +
> 
> Since you have to do a v3, please make this part (which is common
> between HMP and QMP) a separate function.

Also please make qemu_chr_new_from_opts use an Error ** at least for the
error that are reported from that function itself (no need to add
propagation to all of qemu-char.c, and it would conflict badly with the
series that does so for qemu-sockets.c).

Paolo

On 10/12/2012 08:39 PM, Gerd Hoffmann wrote:
> This patch adds chardev_add and chardev_del monitor commands.
>
> chardev_del is pretty straight forward, it just takes an id argument and
> zaps the chardev specified.
>
> chardev_add is more tricky as there are tons of arguments for the
> different backends.  The hmp version limited to the most common use
> cases, especially when it comes to sockets:  You can only specify port
> (tcp) or path (unix) and qemu will create a listening socket.  For
> example this ...
>
>     (qemu) chardev_add foo socket 42
>
> ... will do the same as ...
>
>     -chardev socket,id=foo,port=42,server,nowait
>
> on the qemu command line.
>
> The qmp version has full support for everything the -chardev command
> line switch can handle.  The implementation is pretty straight
> forward: It just puts all arguments it got into a QemuOpts, then goes
> call qemu_chr_new_from_opts().
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hmp-commands.hx  |   32 ++++++++++++++++++++++++++++
>   hmp.c            |   31 +++++++++++++++++++++++++++
>   hmp.h            |    2 +
>   qapi-schema.json |   39 ++++++++++++++++++++++++++++++++++
>   qemu-char.c      |   50 +++++++++++++++++++++++++++++++++++++++++++-
>   qemu-char.h      |    2 +
>   qmp-commands.hx  |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   7 files changed, 216 insertions(+), 1 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e0b537d..48504d1 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1404,6 +1404,38 @@ passed since 1970, i.e. unix epoch.
>   ETEXI
>
>       {
> +        .name       = "chardev_add",
> +        .args_type  = "args:s",
> +        .params     = "args",
> +        .help       = "add chardev",
> +        .mhandler.cmd = hmp_chardev_add,
> +    },
> +
> +STEXI
> +@item chardev_add args
> +@findex chardev_add
> +
> +chardev_add accepts the same parameters as the -chardev command line switch.
> +
> +ETEXI
> +
> +    {
> +        .name       = "chardev_del",
> +        .args_type  = "id:s",
> +        .params     = "id",
> +        .help       = "del chardev",
> +        .mhandler.cmd = hmp_chardev_del,
> +    },
> +
> +STEXI
> +@item chardev_del id
> +@findex chardev_del
> +
> +Removes the chardev @var{id}.
> +
> +ETEXI
> +
> +    {
>           .name       = "info",
>           .args_type  = "item:s?",
>           .params     = "[subcommand]",
> diff --git a/hmp.c b/hmp.c
> index 70bdec2..b494d05 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1209,3 +1209,34 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>       qmp_screendump(filename, &err);
>       hmp_handle_error(mon, &err);
>   }
> +
> +void hmp_chardev_add(Monitor *mon, const QDict *qdict)
> +{
> +    const char *args = qdict_get_str(qdict, "args");
> +    CharDriverState *chr;
> +    Error *err = NULL;
> +    QemuOpts *opts;
> +
> +    opts = qemu_opts_parse(qemu_find_opts("chardev"), args, 1);
> +    if (opts == NULL) {
> +        error_setg(&err, "Parsing chardev args failed\n");
> +        goto out;
> +    }
> +
> +    chr = qemu_chr_new_from_opts(opts, NULL);
> +    if (chr == NULL) {
> +        qemu_opts_del(opts);
> +        error_setg(&err, "Creating chardev failed\n");
> +    }
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}
> +
> +void hmp_chardev_del(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    qmp_chardev_del(qdict_get_str(qdict, "id"),
> +                    &err);
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 71ea384..080afaa 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
>   void hmp_closefd(Monitor *mon, const QDict *qdict);
>   void hmp_send_key(Monitor *mon, const QDict *qdict);
>   void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> +void hmp_chardev_add(Monitor *mon, const QDict *qdict);
> +void hmp_chardev_del(Monitor *mon, const QDict *qdict);
>
>   #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f9dbdae..550e4c7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2796,3 +2796,42 @@
>   # Since: 0.14.0
>   ##
>   { 'command': 'screendump', 'data': {'filename': 'str'} }
> +
> +##
> +# @chardev_add:
> +#
> +# Add a chardev
> +#
> +# @id: the chardev's ID, must be unique
> +# @backend: the chardev backend: "file", "socket", ...
> +# @path: file / device / unix socket path
> +# @name: spice channel name
> +# @host: host name
> +# @port: port number
> +# @server: create socket in server mode
> +# @wait: wait for connect
> +# @ipv4: force ipv4-only
> +# @ipv6: force ipv6-only
> +# @telnet: telnet negotiation
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'chardev_add', 'data': {'id'      : 'str',
> +                                     'backend' : 'str',
> +                                     '*props'  : '**' },
> +  'gen': 'no' }
> +
> +##
> +# @chardev_del:
> +#
> +# Remove a chardev
> +#
> +# @id: the chardev's ID, must exist and not be in use
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.3.0
> +##
> +{ 'command': 'chardev_del', 'data': {'id': 'str'} }
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..7bbc490 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2805,6 +2805,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>           chr->avail_connections = 1;
>       }
>       chr->label = g_strdup(qemu_opts_id(opts));
> +    chr->opts = opts;
>       return chr;
>   }
>
> @@ -2826,7 +2827,6 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
>       if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
>           monitor_init(chr, MONITOR_USE_READLINE);
>       }
> -    qemu_opts_del(opts);
>       return chr;
>   }
>
> @@ -2856,6 +2856,7 @@ void qemu_chr_delete(CharDriverState *chr)
>       QTAILQ_REMOVE(&chardevs, chr, next);
>       if (chr->chr_close)
>           chr->chr_close(chr);
> +    qemu_opts_del(chr->opts);
>       g_free(chr->filename);
>       g_free(chr->label);
>       g_free(chr);
> @@ -2900,3 +2901,50 @@ CharDriverState *qemu_char_get_next_serial(void)
>       return serial_hds[next_serial++];
>   }
>
> +int qmp_chardev_add(Monitor *mon, const QDict *qdict, QObject **ret)
> +{
> +    CharDriverState *chr;
> +    Error *err = NULL;
> +    QemuOptsList *opts_list;
> +    QemuOpts *opts;
> +
> +    opts_list = qemu_find_opts_err("chardev", &err);
> +    if (error_is_set(&err)) {
> +        goto exit_err;
> +    }
> +
> +    opts = qemu_opts_from_qdict(opts_list, qdict, &err);
> +    if (error_is_set(&err)) {
> +        goto exit_err;
> +    }
> +
> +    chr = qemu_chr_new_from_opts(opts, NULL);
> +    if (chr == NULL) {
> +        qemu_opts_del(opts);
> +        error_setg(&err, "Creating chardev failed\n");
> +        goto exit_err;
> +    }
> +    return 0;
> +
> +exit_err:
> +    qerror_report_err(err);
> +    error_free(err);
> +    return -1;
> +}
> +
> +void qmp_chardev_del(const char *id, Error **errp)
> +{
> +    CharDriverState *chr;
> +
> +    chr = qemu_chr_find(id);
> +    if (NULL == chr) {
> +        error_setg(errp, "Chardev '%s' not found\n", id);

Maybe this should be replaced by QERR_ macros to keep
compatibility since this one is listed in ErrorClass
in the schema, like:

error_set(errp, QERR_DEVICE_NOT_FOUND, id);

> +        return;
> +    }
> +    if (chr->chr_can_read || chr->chr_read ||
> +        chr->chr_event || chr->handler_opaque) {
> +        error_setg(errp, "Chardev '%s' is busy\n", id);
> +        return;
> +    }
> +    qemu_chr_delete(chr);
> +}
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..a910a60 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -74,6 +74,7 @@ struct CharDriverState {
>       char *filename;
>       int opened;
>       int avail_connections;
> +    QemuOpts *opts;
>       QTAILQ_ENTRY(CharDriverState) next;
>   };
>
> @@ -236,6 +237,7 @@ void qemu_chr_info(Monitor *mon, QObject **ret_data);
>   CharDriverState *qemu_chr_find(const char *name);
>
>   QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
> +int qmp_chardev_add(Monitor *mon, const QDict *qdict, QObject **ret);
>
>   /* add an eventfd to the qemu devices that are polled */
>   CharDriverState *qemu_chr_open_eventfd(int eventfd);
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2f8477e..ebc1cee 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2576,3 +2576,64 @@ EQMP
>           .args_type  = "",
>           .mhandler.cmd_new = qmp_marshal_input_query_target,
>       },
> +
> +    {
> +        .name       = "chardev_add",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_chardev_add,
> +    },
> +
> +SQMP
> +chardev_add
> +-----------
> +
> +Add a chardev.
> +
> +Arguments:
> +
> +- "id": the chardev's ID, must be unique (json-string)
> +- "backend": the chardev backend: "file", "socket", ... (json-string)
> +- "path": file / device / unix socket path (json-string, optional)
> +- "name": spice channel name (json-string, optional)
> +- "host": host name (json-string, optional)
> +- "port": port number (json-string, optional)
> +- "server": create socket in server mode (json-bool, optional)
> +- "wait": wait for connect (json-bool, optional)
> +- "ipv4": force ipv4-only (json-bool, optional)
> +- "ipv6": force ipv6-only (json-bool, optional)
> +- "telnet": telnet negotiation (json-bool, optional)
> +

Should it support the option "mux" to enable multiplexing mode?

> +Example:
> +
> +-> { "execute": "chardev_add", "arguments": { "id"      : "foo",
> +                                              "backend" : "socket",
> +                                              "path"    : "/tmp/foo",
> +                                              "server"  : "on",
> +                                              "wait"    : "off" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "chardev_del",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_chardev_del,
> +    },
> +
> +
> +SQMP
> +chardev_del
> +-----------
> +
> +Remove a chardev.
> +
> +Arguments:
> +
> +- "id": the chardev's ID, must exist and not be in use (json-string)
> +
> +Example:
> +
> +-> { "execute": "chardev_del", "arguments": { "id" : "foo" } }
> +<- { "return": {} }
> +
> +EQMP

Am 15.10.2012 08:51, schrieb Lei Li:
> On 10/12/2012 08:39 PM, Gerd Hoffmann wrote:
>> +void qmp_chardev_del(const char *id, Error **errp)
>> +{
>> +    CharDriverState *chr;
>> +
>> +    chr = qemu_chr_find(id);
>> +    if (NULL == chr) {
>> +        error_setg(errp, "Chardev '%s' not found\n", id);
> 
> Maybe this should be replaced by QERR_ macros to keep
> compatibility since this one is listed in ErrorClass
> in the schema, like:
> 
> error_set(errp, QERR_DEVICE_NOT_FOUND, id);

No, error_setg() is the new replacement, QERR_* is deprecated.

Andreas

On 10/15/2012 12:51 AM, Lei Li wrote:
> On 10/12/2012 08:39 PM, Gerd Hoffmann wrote:
>> This patch adds chardev_add and chardev_del monitor commands.
>>
>> chardev_del is pretty straight forward, it just takes an id argument and
>> zaps the chardev specified.
>>
>> chardev_add is more tricky as there are tons of arguments for the
>> different backends.  The hmp version limited to the most common use
>> cases, especially when it comes to sockets:  You can only specify port
>> (tcp) or path (unix) and qemu will create a listening socket.  For
>> example this ...
>>
>>     (qemu) chardev_add foo socket 42
>>
>> ... will do the same as ...
>>
>>     -chardev socket,id=foo,port=42,server,nowait
>>
>> on the qemu command line.
>>
>> The qmp version has full support for everything the -chardev command
>> line switch can handle.  The implementation is pretty straight
>> forward: It just puts all arguments it got into a QemuOpts, then goes
>> call qemu_chr_new_from_opts().
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

>> +++ b/qapi-schema.json
>> @@ -2796,3 +2796,42 @@
>>   # Since: 0.14.0
>>   ##
>>   { 'command': 'screendump', 'data': {'filename': 'str'} }
>> +
>> +##
>> +# @chardev_add:

The QMP command should be named 'chardev-add'.

>> +#
>> +# Add a chardev
>> +#
>> +# @id: the chardev's ID, must be unique
>> +# @backend: the chardev backend: "file", "socket", ...

Rather than making this an open-coded string, should it instead be a QMP
enum value?

>> +##
>> +# @chardev_del:

And this should be 'chardev-del' or even 'chardev-remove', as QMP
commands tend to favor legibility over abbreviations.

On Mon, 15 Oct 2012 13:09:55 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 15.10.2012 08:51, schrieb Lei Li:
> > On 10/12/2012 08:39 PM, Gerd Hoffmann wrote:
> >> +void qmp_chardev_del(const char *id, Error **errp)
> >> +{
> >> +    CharDriverState *chr;
> >> +
> >> +    chr = qemu_chr_find(id);
> >> +    if (NULL == chr) {
> >> +        error_setg(errp, "Chardev '%s' not found\n", id);
> > 
> > Maybe this should be replaced by QERR_ macros to keep
> > compatibility since this one is listed in ErrorClass
> > in the schema, like:
> > 
> > error_set(errp, QERR_DEVICE_NOT_FOUND, id);
> 
> No, error_setg() is the new replacement, QERR_* is deprecated.

This is a case where the error appears in the ErrorClass enum, in this
case we're supposed to use the QERR_ macro for compatibility in the wire.

On the other hand, this is a new command so I'm not completely sure
this matters (in doubt we should do it though).

Il 15/10/2012 19:36, Eric Blake ha scritto:
>>> >> +##
>>> >> +# @chardev_del:
> And this should be 'chardev-del' or even 'chardev-remove', as QMP
> commands tend to favor legibility over abbreviations.

In doubt we should favor consistency though (with netdev_add and
netdev_del).  I'm for keeping this as Gerd did, and perhaps implement
automatic _ <-> - conversion for QMP.

Paolo