Re: [Qemu-devel] [PATCH v2 3/9] char: chardevice hotswap

[Anton Nefedov <anton.nefedov@virtuozzo.com>]

> 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