[1/2] char: chardevice hotswap

Commit Message

Denis V. Lunev Feb. 9, 2017, 3:25 p.m. UTC

From: Anton Nefedov <[e-mail]>

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 <[e-mail]>
Signed-off-by: Denis V. Lunev <[e-mail]>
CC: Paolo Bonzini <[e-mail]>
CC: "Marc-André Lureau" <[e-mail]>
CC: "Dr. David Alan Gilbert" <[e-mail]>
---
 chardev/char.c        | 161 +++++++++++++++++++++++++++++++++++++++++++++++---
 hmp.c                 |  14 +++++
 include/sysemu/char.h |  25 ++++++++
 3 files changed, 191 insertions(+), 9 deletions(-)

Comments

Daniel P. Berrangé Feb. 9, 2017, 4:09 p.m. UTC | #1

On Thu, Feb 09, 2017 at 06:25:33PM +0300, Denis V. Lunev wrote:
> From: Anton Nefedov <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Denis V. Lunev <[e-mail]>
> CC: Paolo Bonzini <[e-mail]>
> CC: "Marc-André Lureau" <[e-mail]>
> CC: "Dr. David Alan Gilbert" <[e-mail]>
> ---
>  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

Dr. David Alan Gilbert Feb. 9, 2017, 4:43 p.m. UTC | #2

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

Patch

diff --git a/chardev/char.c b/chardev/char.c
index abd525f..ec52b52 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -136,12 +136,16 @@  static bool qemu_chr_replay(Chardev *chr)
 
 int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
 {
-    Chardev *s = be->chr;
+    Chardev *s;
     ChardevClass *cc;
     int ret;
 
+    qemu_mutex_lock(&be->chr_lock);
+    s = be->chr;
+
     if (!s) {
-        return 0;
+        ret = 0;
+        goto end;
     }
 
     if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_PLAY) {
@@ -149,7 +153,7 @@  int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
         replay_char_write_event_load(&ret, &offset);
         assert(offset <= len);
         qemu_chr_fe_write_buffer(s, buf, offset, &offset);
-        return ret;
+        goto end;
     }
 
     cc = CHARDEV_GET_CLASS(s);
@@ -165,7 +169,9 @@  int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
     if (qemu_chr_replay(s) && replay_mode == REPLAY_MODE_RECORD) {
         replay_char_write_event_save(ret, ret < 0 ? 0 : ret);
     }
-    
+
+end:
+    qemu_mutex_unlock(&be->chr_lock);
     return ret;
 }
 
@@ -195,13 +201,16 @@  int qemu_chr_write_all(Chardev *s, const uint8_t *buf, int len)
 
 int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len)
 {
-    Chardev *s = be->chr;
+    Chardev *s;
+    int ret;
 
-    if (!s) {
-        return 0;
-    }
+    qemu_mutex_lock(&be->chr_lock);
+
+    s = be->chr;
+    ret = s ? qemu_chr_write_all(s, buf, len) : 0;
 
-    return qemu_chr_write_all(s, buf, len);
+    qemu_mutex_unlock(&be->chr_lock);
+    return ret;
 }
 
 int qemu_chr_fe_read_all(CharBackend *be, uint8_t *buf, int len)
@@ -505,6 +514,10 @@  bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
 
     b->fe_open = false;
     b->tag = tag;
+
+    if (!b->chr) {
+        qemu_mutex_init(&b->chr_lock);
+    }
     b->chr = s;
     return true;
 
@@ -537,9 +550,22 @@  void qemu_chr_fe_deinit(CharBackend *b)
             d->backends[b->tag] = NULL;
         }
         b->chr = NULL;
+        qemu_mutex_destroy(&b->chr_lock);
+        if (b->hotswap_bh) {
+            qemu_bh_delete(b->hotswap_bh);
+        }
     }
 }
 
+void qemu_chr_fe_set_be_change_handler(CharBackend *b,
+                                       BackendChangeHandler *be_change)
+{
+    if (!b->chr) {
+        return;
+    }
+    b->chr_be_change = be_change;
+}
+
 void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
                               IOReadHandler *fd_read,
@@ -875,6 +901,123 @@  help_string_append(const char *name, void *opaque)
     g_string_append_printf(str, "\n%s", name);
 }
 
+#define CHARDEV_CHANGE_PREFIX "tmp-chardev-xchg-"
+
+static void qemu_chr_change_bh(void *opaque)
+{
+    char *id = opaque;
+    char *tmpid = g_strdup_printf(CHARDEV_CHANGE_PREFIX "%s", id);
+    Chardev *chr = qemu_chr_find(id);
+    Chardev *chr_new = qemu_chr_find(tmpid);
+    CharBackend *be = chr->be;
+    bool closed_sent = false;
+
+    if (be) {
+        if (chr->be_open && !chr_new->be_open) {
+            qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+            closed_sent = true;
+        }
+
+        qemu_mutex_lock(&be->chr_lock);
+        qemu_chr_fe_init(be, chr_new, &error_abort);
+
+        if (be->chr_be_change(be->opaque) < 0) {
+            fprintf(stderr, "Chardev '%s' change failed", id);
+            qemu_chr_fe_init(be, chr, &error_abort);
+            qemu_mutex_unlock(&be->chr_lock);
+            if (closed_sent) {
+                qemu_chr_be_event(chr, CHR_EVENT_OPENED);
+            }
+            qemu_chr_delete(chr_new);
+            g_free(id);
+            g_free(tmpid);
+            return;
+        }
+        qemu_mutex_unlock(&be->chr_lock);
+
+        chr->be = NULL;
+    }
+
+    qemu_chr_delete(chr);
+    g_free(chr_new->label);
+    chr_new->label = id;
+    g_free(tmpid);
+}
+
+void qemu_chr_change(QemuOpts *opts, Error **errp)
+{
+    const char *be_name = qemu_opt_get(opts, "backend");
+    const char *id = qemu_opts_id(opts);
+    char *tmpid;
+    Chardev *chr, *chr_new;
+    const ChardevClass *cc;
+    int i;
+
+    if (!id) {
+        error_setg(errp, "chardev: no id specified");
+        return;
+    }
+
+    chr = qemu_chr_find(id);
+    if (!chr) {
+        error_setg(errp, "Chardev '%s' does not exist", id);
+        return;
+    }
+
+    if (!be_name) {
+        error_setg(errp, "chardev: '%s' missing backend", id);
+        return;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(chardev_alias_table); i++) {
+        if (g_strcmp0(chardev_alias_table[i].alias, be_name) == 0) {
+            be_name = chardev_alias_table[i].typename;
+            break;
+        }
+    }
+
+    cc = char_get_class(be_name, errp);
+    if (!cc) {
+        return;
+    }
+
+    if (chr->be && !chr->be->chr_be_change) {
+        error_setg(errp, "Chardev user does not support chardev hotswap");
+        return;
+    }
+
+    if (qemu_chr_replay(chr)) {
+        error_setg(errp,
+            "Chardev '%s' cannot be changed in record/replay mode", id);
+        return;
+    }
+
+    if (CHARDEV_IS_MUX(chr)) {
+        error_setg(errp, "Mux device hotswap not supported yet");
+        return;
+    }
+
+    tmpid = g_strdup_printf(CHARDEV_CHANGE_PREFIX "%s", id);
+    qemu_opts_set_id(opts, tmpid);
+    chr_new = qemu_chr_new_from_opts(opts, errp);
+    qemu_opts_set_id(opts, (char *)id);
+    g_free(tmpid);
+
+    if (!chr_new) {
+        return;
+    }
+
+    if (chr->be) {
+        if (chr->be->hotswap_bh) {
+            qemu_bh_delete(chr->be->hotswap_bh);
+        }
+        chr->be->hotswap_bh = qemu_bh_new(qemu_chr_change_bh, g_strdup(id));
+        qemu_bh_schedule(chr->be->hotswap_bh);
+    } else {
+        qemu_chr_change_bh(g_strdup(id));
+    }
+}
+
 Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
                                 Error **errp)
 {
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);
+        }
     } else {
         if (read_only) {
             read_only_mode =
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 450881d..3e99953 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -56,6 +56,7 @@  struct ParallelIOArg {
 #define CHR_TIOCM_RTS	0x004
 
 typedef void IOEventHandler(void *opaque, int event);
+typedef int BackendChangeHandler(void *opaque);
 
 typedef enum {
     /* Whether the chardev peer is able to close and
@@ -79,9 +80,12 @@  typedef struct CharBackend {
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
+    BackendChangeHandler *chr_be_change;
     void *opaque;
     int tag;
     int fe_open;
+    QemuMutex chr_lock;
+    QEMUBH *hotswap_bh;
 } CharBackend;
 
 struct Chardev {
@@ -132,6 +136,14 @@  void qemu_chr_parse_common(QemuOpts *opts, ChardevCommon *backend);
  */
 Chardev *qemu_chr_new(const char *label, const char *filename);
 
+/**
+ * @qemu_chr_change:
+ *
+ * Change an existing character backend
+ *
+ * @opts the new backend options
+ */
+void qemu_chr_change(QemuOpts *opts, Error **errp);
 
 /**
  * @qemu_chr_fe_disconnect:
@@ -419,6 +431,19 @@  void qemu_chr_fe_set_handlers(CharBackend *b,
                               bool set_open);
 
 /**
+ * @qemu_chr_fe_set_be_change_handler:
+ * @be_change: backend change callback
+ *
+ * Set the front end handler for the backend hotswap.
+ * If the handler is not set or set to NULL, no backend hotswap will
+ * be performed.
+ *
+ * Without associated Chardev, nothing is changed.
+ */
+void qemu_chr_fe_set_be_change_handler(CharBackend *b,
+                                       BackendChangeHandler *be_change);
+
+/**
  * @qemu_chr_fe_take_focus:
  *
  * Take the focus (if the front end is muxed).