Hi On Tue, May 30, 2017 at 6:12 PM Anton Nefedov <anton.nefe...@virtuozzo.com> wrote:
> This patch adds a possibility to change a char device without a frontend > removal. > > 1. Ideally, it would have to happen transparently to a frontend, i.e. > frontend would continue its regular operation. > However, backends are not stateless 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. > > 2. 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. > > Tbh, I don't understand the need for a different lock. Could you explain? Even better would be to write a test that shows in which way the lock helps. Signed-off-by: Anton Nefedov <anton.nefe...@virtuozzo.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > patch looks good otherwise > --- > chardev/char.c | 126 > ++++++++++++++++++++++++++++++++++++++++++++++---- > include/sysemu/char.h | 9 ++++ > qapi-schema.json | 40 ++++++++++++++++ > 3 files changed, 165 insertions(+), 10 deletions(-) > > diff --git a/chardev/char.c b/chardev/char.c > index 1b097b3..ba1a5f5 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -132,12 +132,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) { > @@ -145,7 +149,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); > @@ -161,7 +165,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; > } > > @@ -191,13 +197,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) > @@ -478,7 +487,7 @@ Chardev *qemu_chr_fe_get_driver(CharBackend *be) > return be->chr; > } > > -bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp) > +static bool qemu_chr_fe_connect(CharBackend *b, Chardev *s, Error **errp) > { > int tag = 0; > > @@ -507,6 +516,16 @@ unavailable: > return false; > } > > +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp) > +{ > + if (!qemu_chr_fe_connect(b, s, errp)) { > + return false; > + } > + > + qemu_mutex_init(&b->chr_lock); > + return true; > +} > + > static bool qemu_chr_is_busy(Chardev *s) > { > if (CHARDEV_IS_MUX(s)) { > @@ -531,6 +550,7 @@ void qemu_chr_fe_deinit(CharBackend *b) > d->backends[b->tag] = NULL; > } > b->chr = NULL; > + qemu_mutex_destroy(&b->chr_lock); > } > } > > @@ -1306,6 +1326,92 @@ ChardevReturn *qmp_chardev_add(const char *id, > ChardevBackend *backend, > return ret; > } > > +ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend, > + Error **errp) > +{ > + CharBackend *be; > + const ChardevClass *cc; > + Chardev *chr, *chr_new; > + bool closed_sent = false; > + ChardevReturn *ret; > + > + chr = qemu_chr_find(id); > + if (!chr) { > + error_setg(errp, "Chardev '%s' does not exist", id); > + return NULL; > + } > + > + if (CHARDEV_IS_MUX(chr)) { > + error_setg(errp, "Mux device hotswap not supported yet"); > + return NULL; > + } > + > + if (qemu_chr_replay(chr)) { > + error_setg(errp, > + "Chardev '%s' cannot be changed in record/replay mode", id); > + return NULL; > + } > + > + be = chr->be; > + if (!be) { > + /* easy case */ > + object_unparent(OBJECT(chr)); > + return qmp_chardev_add(id, backend, errp); > + } > + > + if (!be->chr_be_change) { > + error_setg(errp, "Chardev user does not support chardev hotswap"); > + return NULL; > + } > + > + cc = char_get_class(ChardevBackendKind_lookup[backend->type], errp); > + if (!cc) { > + return NULL; > + } > + > + chr_new = qemu_chardev_new(NULL, > object_class_get_name(OBJECT_CLASS(cc)), > + backend, errp); > + if (!chr_new) { > + return NULL; > + } > + chr_new->label = g_strdup(id); > + > + 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); > + chr->be = NULL; > + qemu_chr_fe_connect(be, chr_new, &error_abort); > + > + if (be->chr_be_change(be->opaque) < 0) { > + error_setg(errp, "Chardev '%s' change failed", chr_new->label); > + chr_new->be = NULL; > + qemu_chr_fe_connect(be, chr, &error_abort); > + qemu_mutex_unlock(&be->chr_lock); > + if (closed_sent) { > + qemu_chr_be_event(chr, CHR_EVENT_OPENED); > + } > + object_unref(OBJECT(chr_new)); > + return NULL; > + } > + qemu_mutex_unlock(&be->chr_lock); > + > + object_unparent(OBJECT(chr)); > + object_property_add_child(get_chardevs_root(), chr_new->label, > + OBJECT(chr_new), &error_abort); > + object_unref(OBJECT(chr_new)); > + > + ret = g_new0(ChardevReturn, 1); > + if (CHARDEV_IS_PTY(chr_new)) { > + ret->pty = g_strdup(chr_new->filename + 4); > + ret->has_pty = true; > + } > + > + return ret; > +} > + > void qmp_chardev_remove(const char *id, Error **errp) > { > Chardev *chr; > diff --git a/include/sysemu/char.h b/include/sysemu/char.h > index 9f8df07..014ebbc 100644 > --- a/include/sysemu/char.h > +++ b/include/sysemu/char.h > @@ -92,6 +92,7 @@ typedef struct CharBackend { > void *opaque; > int tag; > int fe_open; > + QemuMutex chr_lock; > } CharBackend; > > struct Chardev { > @@ -141,6 +142,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: > diff --git a/qapi-schema.json b/qapi-schema.json > index e38c5f0..0f0df36 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -5097,6 +5097,46 @@ > 'returns': 'ChardevReturn' } > > ## > +# @chardev-change: > +# > +# Change a character device backend > +# > +# @id: the chardev's ID, must exist > +# @backend: new backend type and parameters > +# > +# Returns: ChardevReturn. > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute" : "chardev-change", > +# "arguments" : { "id" : "baz", > +# "backend" : { "type" : "pty", "data" : {} } } } > +# <- { "return": { "pty" : "/dev/pty/42" } } > +# > +# -> {"execute" : "chardev-change", > +# "arguments" : { > +# "id" : "charchannel2", > +# "backend" : { > +# "type" : "socket", > +# "data" : { > +# "addr" : { > +# "type" : "unix" , > +# "data" : { > +# "path" : "/tmp/charchannel2.socket" > +# } > +# }, > +# "server" : true, > +# "wait" : false }}}} > +# <- {"return": {}} > +# > +## > +{ 'command': 'chardev-change', 'data': {'id' : 'str', > + 'backend' : 'ChardevBackend' }, > + 'returns': 'ChardevReturn' } > + > +## > # @chardev-remove: > # > # Remove a character device backend > -- > 2.7.4 > > > -- Marc-André Lureau