[v2,3/9] char: chardevice hotswap

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.

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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/char.c        | 147 ++++++++++++++++++++++++++++++++++++++++++++++----
 include/sysemu/char.h |  10 ++++
 qapi-schema.json      |  40 ++++++++++++++
 3 files changed, 187 insertions(+), 10 deletions(-)

Hi

On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <anton.nefedov@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.
>
>
Why not use the chr_write_lock then? (rename it chr_lock?)


> 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)
>

What kind of troubles?

>
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  chardev/char.c        | 147
> ++++++++++++++++++++++++++++++++++++++++++++++----
>  include/sysemu/char.h |  10 ++++
>  qapi-schema.json      |  40 ++++++++++++++
>  3 files changed, 187 insertions(+), 10 deletions(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index ae60950..bac5e1c 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);
>
> -    return qemu_chr_write_all(s, buf, len);
> +    s = be->chr;
> +    ret = s ? qemu_chr_write_all(s, buf, len) : 0;
> +
> +    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 fe_connect(CharBackend *b, Chardev *s, Error **errp)
>  {
>

I would rather keep a qemu_chr prefix for consistency


>      int tag = 0;
>
> @@ -507,6 +516,17 @@ unavailable:
>      return false;
>  }
>
> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
> +{
> +    if (!fe_connect(b, s, errp)) {
> +        return false;
> +    }
> +
> +    qemu_mutex_init(&b->chr_lock);
> +    b->hotswap_bh = NULL;
> +    return true;
> +}
> +
>  static bool qemu_chr_is_busy(Chardev *s)
>  {
>      if (CHARDEV_IS_MUX(s)) {
> @@ -531,6 +551,10 @@ 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);
>

Could there be a chr_new leak here?


> +        }
>      }
>  }
>
> @@ -1308,6 +1332,109 @@ ChardevReturn *qmp_chardev_add(const char *id,
> ChardevBackend *backend,
>      return ret;
>  }
>
> +static void chardev_change_bh(void *opaque)
> +{
> +    Chardev *chr_new = opaque;
> +    const char *id = chr_new->label;
> +    Chardev *chr = qemu_chr_find(id);
>

Could chr be gone in the meantime? potentially


> +    CharBackend *be = chr->be;
> +    bool closed_sent = false;
> +
> +    if (!be) {
> +        /* disconnected since we checked: ok, less work for us */
> +        goto end;
> +    }
> +
> +    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;
> +    fe_connect(be, chr_new, &error_abort);
> +
> +    if (be->chr_be_change(be->opaque) < 0) {
> +        error_report("Chardev '%s' change failed", id);
> +        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;
> +    }
> +    qemu_mutex_unlock(&be->chr_lock);
> +
> +end:
> +    object_unparent(OBJECT(chr));
> +    object_property_add_child(get_chardevs_root(), id, OBJECT(chr_new),
> +                              &error_abort);
> +    object_unref(OBJECT(chr_new));
> +}
> +
> +ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
> +                                  Error **errp)
> +{
> +    const ChardevClass *cc;
> +    Chardev *chr, *chr_new;
> +    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;
> +    }
> +
> +    if (!chr->be) {
> +        /* easy case */
> +        object_unparent(OBJECT(chr));
> +        return qmp_chardev_add(id, backend, errp);
> +    }
> +
> +    if (!chr->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);
> +    chr_new->label = g_strdup(id);
>

move it below the check for !chr_new


> +    if (!chr_new) {
> +        return NULL;
> +    }
> +
> +    if (chr->be->hotswap_bh) {
> +        qemu_bh_delete(chr->be->hotswap_bh);
> +    }
>

Is it necessary to delete and recreate the bh? Could there be a chr_new
leak if the bh didn't run? It's probably best to deny backend change while
one is going on.


> +    chr->be->hotswap_bh = qemu_bh_new(chardev_change_bh, chr_new);
> +    qemu_bh_schedule(chr->be->hotswap_bh);
> +
> +    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;
>

ah, potentially a case where qmp-async would be better..

+}
> +
>  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..68c7876 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -92,6 +92,8 @@ typedef struct CharBackend {
>      void *opaque;
>      int tag;
>      int fe_open;
> +    QemuMutex chr_lock;
> +    QEMUBH *hotswap_bh;
>  } CharBackend;
>
>  struct Chardev {
> @@ -141,6 +143,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);
>
>
This patch needs some test-char.c tests.


>  /**
>   * @qemu_chr_fe_disconnect:
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 80603cf..bf72166 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5075,6 +5075,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

> Hi
> 

Hi Marc-André, thanks for reviewing, pls see answers below

> On Fri, May 19, 2017 at 4:51 PM Anton Nefedov <address@hidden>
> 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.
>>
>>
> Why not use the chr_write_lock then? (rename it chr_lock?)
> 

chr_write_lock is a Chardev property, it will be gone together with a
swapped device.

We could remove chr_write_lock and leave only the new be->chr_lock, 
since it covers everything; I guess the only problem is mux, where
multiple CharBackends share the same Chardev.

Would smth like this be better?:

static void char_lock(CharBackend *be)
{
     qemu_mutex_lock(be->chr && CHARDEV_IS_MUX(be->chr) ?
                     &be->chr->chr_lock : &be->chr_lock);
}

and use this instead of lock(be->chr_lock) and remove chr_write_lock.
(This would rely on a fact that mux is never hotswapped so be->chr_lock
is not needed).


> 
>> 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)
>>
> 
> What kind of troubles?
> 

if someone tried to hotswap monitor, it would look like:

(gdb) bt
#0 qmp_chardev_change (id=id@entry=0x7fc24bee7a60 "charmonitor",
backend=backend@entry=0x7fc24cc28290, errp=errp@entry=0x7ffde4d8d4f0)
at /mnt/code/us-qemu/chardev/char.c:1438
#1 0x00007fc249fd914b in hmp_chardev_change (mon=<optimized out>,
qdict=<optimized out>) at /mnt/code/us-qemu/hmp.c:2252
#2 0x00007fc249edeece in handle_hmp_command (mon=mon@entry=0x7fc24bef87c0,
cmdline=0x7fc24bf2345f "charmonitor
socket,path=/vz/vmprivate/centosos/mon.socket,server,nowait") at
/mnt/code/us-qemu/monitor.c:3100
#3 0x00007fc249ee02fa in monitor_command_cb (opaque=0x7fc24bef87c0,
cmdline=<optimized out>, readline_opaque=<optimized out>)
at /mnt/code/us-qemu/monitor.c:3897
#4 0x00007fc24a242428 in readline_handle_byte (rs=0x7fc24bf23450,
ch=<optimized out>) at /mnt/code/us-qemu/util/readline.c:393
#5 0x00007fc249edf0e2 in monitor_read (opaque=<optimized out>,
buf=<optimized out>, size=<optimized out>)
at /mnt/code/us-qemu/monitor.c:3880
#6 0x00007fc24a1deb2b in tcp_chr_read (chan=<optimized out>,
cond=<optimized out>, opaque=<optimized out>)
at /mnt/code/us-qemu/chardev/char-socket.c:441
#7 0x00007fc240997d7a in g_main_dispatch (context=0x7fc24beda950) at
gmain.c:3152
#8 g_main_context_dispatch (context=context@entry=0x7fc24beda950) at
gmain.c:3767
#9 0x00007fc24a22fe7c in glib_pollfds_poll () at
/mnt/code/us-qemu/util/main-loop.c:213
#10 os_host_main_loop_wait (timeout=<optimized out>) at
/mnt/code/us-qemu/util/main-loop.c:261
#11 main_loop_wait (nonblocking=nonblocking@entry=0) at
/mnt/code/us-qemu/util/main-loop.c:517
#12 0x00007fc249e9a19a in main_loop () at /mnt/code/us-qemu/vl.c:1900
#13 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized
out>) at /mnt/code/us-qemu/vl.c:4732

i.e. hotswap the device that is in tcp_chr_read (frame #6), and can
potentially be accessed (with old pointer on a stack) after it is
destroyed by qmp_chardev_change()

however, there is no support for monitor hotswap in the patchset. Maybe
it's not worth to mess with bottom-halves at all? It causes problems,
like those mentioned in your further comments


>>
>> Signed-off-by: Anton Nefedov <address@hidden>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>  chardev/char.c        | 147
>> ++++++++++++++++++++++++++++++++++++++++++++++----
>>  include/sysemu/char.h |  10 ++++
>>  qapi-schema.json      |  40 ++++++++++++++
>>  3 files changed, 187 insertions(+), 10 deletions(-)
>>
>> -bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>> +static bool fe_connect(CharBackend *b, Chardev *s, Error **errp)
>>  {
>>
> 
> I would rather keep a qemu_chr prefix for consistency
> 

Done.

> 
>>      int tag = 0;
>>
>> @@ -507,6 +516,17 @@ unavailable:
>>      return false;
>>  }
>>
>> +bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
>> +{
>> +    if (!fe_connect(b, s, errp)) {
>> +        return false;
>> +    }
>> +
>> +    qemu_mutex_init(&b->chr_lock);
>> +    b->hotswap_bh = NULL;
>> +    return true;
>> +}
>> +
>>  static bool qemu_chr_is_busy(Chardev *s)
>>  {
>>      if (CHARDEV_IS_MUX(s)) {
>> @@ -531,6 +551,10 @@ 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);
>>
> 
> Could there be a chr_new leak here?
> 
> 

Yes you're right it can leak.
Maybe we don't need this asynchrony, see above, otherwise I will think
how to fix

>> +        }
>>      }
>>  }
>>
>> @@ -1308,6 +1332,109 @@ ChardevReturn *qmp_chardev_add(const char *id,
>> ChardevBackend *backend,
>>      return ret;
>>  }
>>
>> +static void chardev_change_bh(void *opaque)
>> +{
>> +    Chardev *chr_new = opaque;
>> +    const char *id = chr_new->label;
>> +    Chardev *chr = qemu_chr_find(id);
>>
> 
> Could chr be gone in the meantime? potentially
> 
> 

Potentially yes. Will fix (clean chr_new and bail out).


>> +
>> +    chr_new = qemu_chardev_new(NULL,
>> object_class_get_name(OBJECT_CLASS(cc)),
>> +                               backend, errp);
>> +    chr_new->label = g_strdup(id);
>>
> 
> move it below the check for !chr_new
> 
> 

my bad, thanks

>> +    if (!chr_new) {
>> +        return NULL;
>> +    }
>> +
>> +    if (chr->be->hotswap_bh) {
>> +        qemu_bh_delete(chr->be->hotswap_bh);
>> +    }
>>
> 
> Is it necessary to delete and recreate the bh? Could there be a chr_new
> leak if the bh didn't run? It's probably best to deny backend change while
> one is going on.
> 
> 

Hmm probably not, but new() is the only function that sets the argument.
Will think how to fix this if we decide to keep the bh.

>> +    chr->be->hotswap_bh = qemu_bh_new(chardev_change_bh, chr_new);
>> +    qemu_bh_schedule(chr->be->hotswap_bh);
>> +
>> +    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;
>>
> 
> ah, potentially a case where qmp-async would be better..
> 
> +}
>> +
>>  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..68c7876 100644
>> --- a/include/sysemu/char.h
>> +++ b/include/sysemu/char.h
>> @@ -92,6 +92,8 @@ typedef struct CharBackend {
>>      void *opaque;
>>      int tag;
>>      int fe_open;
>> +    QemuMutex chr_lock;
>> +    QEMUBH *hotswap_bh;
>>  } CharBackend;
>>
>>  struct Chardev {
>> @@ -141,6 +143,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);
>>
>>
> This patch needs some test-char.c tests.
> 
> 

Will do

/Anton