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

[Dr. David Alan Gilbert]

* Daniel P. Berrange (address@hidden) wrote:
> On Thu, Feb 09, 2017 at 06:25:33PM +0300, Denis V. Lunev wrote:
> > From: Anton Nefedov <address@hidden>
> > 
> > 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 <address@hidden>
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Paolo Bonzini <address@hidden>
> > CC: "Marc-André Lureau" <address@hidden>
> > CC: "Dr. David Alan Gilbert" <address@hidden>
> > ---
> >  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.

Hmm.
There's an experimental qmp command - x-blockdev-change (see 7f821597 )
for a failover for block use.

I did post an HMP equivalent 
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg02864.html
which hasn't gone in yet.

So something similar for char might be what you want.

I had wondered if a T adapter for chardevs would be useful in a few situations,
and maybe it would be here; i.e. something where output is sent
to all the outputs, and input is aggregated from the inputs.

Dave

> 
> 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/ :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK