Re: [Qemu-devel] [PATCH] char: allow specifying a GMainContext at opening time

["Daniel P. Berrangé" <berrange@redhat.com>]

On Sat, Feb 02, 2019 at 12:08:34PM +0100, Paolo Bonzini wrote:
> This will be needed by vhost-user-test, when each test switches to
> its own GMainLoop and GMainContext.  Otherwise, for a reconnecting
> socket the initial connection will happen on the default GMainContext,
> and no one will be listening on it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Based-on: <1544684731-18828-1-git-send-email-thuth@redhat.com>

I'm wondering if you had looked at my series related to the chardev
reconnect feature, as this feature has many flaws as implemented
right now:

  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05947.html

Our patches will conflict, but I think yours is needed regardless.

Incidentally I'm increasingly of the opinion that the qemu_chr_new()
design right now is flawed.

IMHO we should really decouple the parsing & creation of the
Chardev object, from the opening of the backend. eg I would like
to see a qemu_chr_open() as a separate explicitly callable step
from qemu_chr_new(). If we had that, then we would not need to
pass GMainContext into qemu_chr_new() as we would be able to
call the API to set the GMainContext before calling qemu_chr_open

I fear that would be a complex refactoring job though, so no
objection to your current patch for solving the immediate need.

> ---
>  chardev/char.c          | 30 +++++++++++++++-------------
>  gdbstub.c               |  4 ++--
>  hmp.c                   |  2 +-
>  hw/arm/omap2.c          |  2 +-
>  hw/bt/hci-csr.c         |  2 +-
>  hw/char/omap_uart.c     |  4 ++--
>  hw/char/xen_console.c   |  3 ++-
>  hw/isa/isa-superio.c    |  4 ++--
>  hw/mips/boston.c        |  2 +-
>  hw/mips/mips_malta.c    |  2 +-
>  hw/usb/dev-serial.c     |  2 +-
>  include/chardev/char.h  | 16 +++++++++++----
>  net/slirp.c             |  2 +-
>  qtest.c                 |  2 +-
>  tests/test-char.c       | 43 +++++++++++++++++++++--------------------
>  tests/vhost-user-test.c |  2 +-
>  vl.c                    |  8 ++++----
>  17 files changed, 72 insertions(+), 58 deletions(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|