Re: [Qemu-devel] [PATCH 1/2] char: chardevice hotswap

["Daniel P. Berrange" <berrange@redhat.com>]

On Thu, Feb 09, 2017 at 06:25:33PM +0300, Denis V. Lunev wrote:
> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
> 
> This patch adds a possibility to change a char device without a frontend
> removal.
> 
> Ideally, it would have to happen transparently to a frontend, i.e. frontend
> would continue its regular operation. However, backends are not stateles
> and are set up by the frontends via qemu_chr_fe_<> functions, and it's not
> (generally) possible to replay that setup entirely in a backend code, as
> different chardevs respond to the setup calls differently, so do frontends
> work differently basing on those setup responses. Moreover, some frontend
> can generally get and save the backend pointer (qemu_chr_fe_get_driver()),
> and it will become invalid after backend change.
> 
> So, a frontend which would like to support chardev hotswap has to register
> a "backend change" handler, and redo its backend setup there.
> 
> Write path can be used by multiple threads and thus protected with
> chr_write_lock. So hotswap also has to be protected so write functions
> won't access a backend being replaced.
> 
> 3. Hotswap function can be called from e.g. a read handler of a monitor
> socket. This can cause troubles so it's safer to defer execution to
> a bottom-half. (however, it means we cannot return some of the errors
> synchronously - but most of them we can)
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> ---
>  chardev/char.c        | 161 +++++++++++++++++++++++++++++++++++++++++++++++---
>  hmp.c                 |  14 +++++

IIRC we required new commands to have a QMP addition and the new HMP
function written by invoking the QMP handler.


> diff --git a/hmp.c b/hmp.c
> index 2bc4f06..70252df 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1522,6 +1522,20 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>              }
>          }
>          qmp_change("vnc", target, !!arg, arg, &err);
> +    } else if (strcmp(device, "chardev") == 0) {
> +        QemuOpts *opts;
> +
> +        if (arg == NULL) {
> +            arg = "";
> +        }
> +        opts = qemu_opts_parse_noisily(qemu_find_opts("chardev"), arg, true);
> +        if (opts == NULL) {
> +            error_setg(&err, "Parsing chardev args failed");
> +        } else {
> +            qemu_opts_set_id(opts, g_strdup(target));
> +            qemu_chr_change(opts, &err);
> +            qemu_opts_del(opts);
> +        }

The hmp 'change' command is/was a huge mistake. We shouldn't continue
to add stuff to it - create dedicated commands for any new functionality
instead of over-loading it one command todo many different things.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|