Re: [Qemu-devel] [PATCH] RFC: chardev: mark the calls that allow an implicit mux monitor

[Eric Blake <eblake@redhat.com>]

On 08/22/2018 01:02 PM, Marc-André Lureau wrote:
> This is mostly for readability of the code. Let's make it clear which
> callers can create an implicit monitor when the chardev is muxed.
> 
> This will also enforce a safer behaviour, as we don't really support
> creating monitor anywhere/anytime at the moment.
> 
> There are documented cases, such as: -serial/-parallel/-virtioconsole
> and to less extent -debugcon.
> 
> Less obvious and questionnable ones are -gdb and Xen console. Add a

s/questionnable/questionable/

> FIXME note for those, but keep the support for now.
> 
> Other qemu_chr_new() callers either have a fixed parameter/filename
> string, or do preliminary checks on the string (such as slirp), or do
> not need it, such as -qtest.
> 
> On a related note, the list of monitor creation places:
> 
> - the chardev creators listed above: all from command line (except
>    perhaps Xen console?)
> 
> - -gdb & hmp gdbserver will create a "GDB monitor command" chardev
>    that is wired to an HMP monitor.
> 
> - -mon command line option
> 
>>From this short study, I would like to think that a monitor may only
> be created in the main thread today, though I remain skeptical :)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/chardev/char.h | 18 +++++++++++++++++-
>   chardev/char.c         | 21 +++++++++++++++++----
>   gdbstub.c              |  6 +++++-
>   hw/char/xen_console.c  |  5 ++++-
>   vl.c                   |  8 ++++----
>   5 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 6f0576e214..be2b9b817e 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -105,6 +105,7 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>    * @qemu_chr_new:
>    *
>    * Create a new character backend from a URI.
> + * Do not implicitely initialize a monitor if the chardev is muxed.

s/implicitely/implicitly/

>    *
>    * @label the name of the backend
>    * @filename the URI
> @@ -113,6 +114,19 @@ ChardevBackend *qemu_chr_parse_opts(QemuOpts *opts,
>    */
>   Chardev *qemu_chr_new(const char *label, const char *filename);
>   
> +/**
> + * @qemu_chr_new_mux_mon:
> + *
> + * Create a new character backend from a URI.
> + * Implicitely initialize a monitor if the chardev is muxed.

and again

> +++ b/gdbstub.c
> @@ -2038,7 +2038,11 @@ int gdbserver_start(const char *device)
>               sigaction(SIGINT, &act, NULL);
>           }
>   #endif
> -        chr = qemu_chr_new_noreplay("gdb", device);
> +        /*
> +         * FIXME: it's a bit weird to allow using a mux chardev here
> +         * and setup implicitely a monitor. We may want to break this.

s/setup implicitely/implicitly set up/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org