Re: [Qemu-devel] [PATCH 2/3] qemu-char: add qemu_chr_be_connected

[Markus Armbruster]

Alon Levy <address@hidden> writes:

> This differs from qemu_chr_be_can_write in that it will return 1 as
> long as the backend is connected, including when the backend is
> connected but not available for receiving, as in the case of
> virtio-serial where the guest ring may be full.
>
> If no implementation is provided the backend reports always connected
> status. This is not really important right now since there is only a
> single user, spice-char-dev.c, which uses virtio-console as the backend,
> which does implement the callback.
>
> Signed-off-by: Alon Levy <address@hidden>
> ---
>  backends/rng-egd.c      |  4 ++--
>  gdbstub.c               |  2 +-
>  hw/ccid-card-passthru.c |  1 +
>  hw/debugcon.c           |  2 +-
>  hw/ivshmem.c            |  8 ++++----
>  hw/qdev-properties.c    |  2 +-
>  hw/serial.c             |  4 ++--
>  hw/usb/dev-serial.c     |  4 ++--
>  hw/usb/redirect.c       |  2 +-
>  hw/virtio-console.c     | 12 ++++++++++--
>  main-loop.h             |  1 +
>  monitor.c               |  4 ++--
>  net/slirp.c             |  2 +-
>  qemu-char.c             | 11 ++++++++++-
>  qemu-char.h             | 13 +++++++++++++
>  qtest.c                 |  2 +-
>  16 files changed, 53 insertions(+), 21 deletions(-)

This patch would be simpler if qemu_chr_add_handlers() took a struct
chr_handlers * pointing to static storage instead of function pointers.
I'm not asking you to make that change.

>
> diff --git a/backends/rng-egd.c b/backends/rng-egd.c
> index ad84737..a20e737 100644
> --- a/backends/rng-egd.c
> +++ b/backends/rng-egd.c
> @@ -150,7 +150,7 @@ static void rng_egd_opened(RngBackend *b, Error **errp)
>      }
>  
>      /* FIXME we should resubmit pending requests when the CDS reconnects. */
> -    qemu_chr_add_handlers(s->chr, rng_egd_chr_can_read, rng_egd_chr_read,
> +    qemu_chr_add_handlers(s->chr, NULL, rng_egd_chr_can_read, 
> rng_egd_chr_read,
>                            NULL, s);
>  }
>  
> @@ -190,7 +190,7 @@ static void rng_egd_finalize(Object *obj)
>      RngEgd *s = RNG_EGD(obj);
>  
>      if (s->chr) {
> -        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> +        qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
>      }
>  
>      g_free(s->chr_name);
> diff --git a/gdbstub.c b/gdbstub.c
> index d02ec75..6b0a2f1 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -3002,7 +3002,7 @@ int gdbserver_start(const char *device)
>          if (!chr)
>              return -1;
>  
> -        qemu_chr_add_handlers(chr, gdb_chr_can_receive, gdb_chr_receive,
> +        qemu_chr_add_handlers(chr, NULL, gdb_chr_can_receive, 
> gdb_chr_receive,
>                                gdb_chr_event, NULL);
>      }
>  
> diff --git a/hw/ccid-card-passthru.c b/hw/ccid-card-passthru.c
> index bd6c777..2734d63 100644
> --- a/hw/ccid-card-passthru.c
> +++ b/hw/ccid-card-passthru.c
> @@ -283,6 +283,7 @@ static int passthru_initfn(CCIDCardState *base)
>      if (card->cs) {
>          DPRINTF(card, D_INFO, "initing chardev\n");
>          qemu_chr_add_handlers(card->cs,
> +            NULL,
>              ccid_card_vscard_can_read,
>              ccid_card_vscard_read,
>              ccid_card_vscard_event, card);
> diff --git a/hw/debugcon.c b/hw/debugcon.c
> index 14ab326..f27ee36 100644
> --- a/hw/debugcon.c
> +++ b/hw/debugcon.c
> @@ -73,7 +73,7 @@ static void debugcon_init_core(DebugconState *s)
>          exit(1);
>      }
>  
> -    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, s);
> +    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, s);
>  }
>  
>  static int debugcon_isa_initfn(ISADevice *dev)
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index f6dbb21..d46ef7c 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -294,10 +294,10 @@ static CharDriverState* create_eventfd_chr_device(void 
> * opaque, EventNotifier *
>          s->eventfd_table[vector].pdev = &s->dev;
>          s->eventfd_table[vector].vector = vector;
>  
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, fake_irqfd,
> +        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, fake_irqfd,
>                        ivshmem_event, &s->eventfd_table[vector]);
>      } else {
> -        qemu_chr_add_handlers(chr, ivshmem_can_receive, ivshmem_receive,
> +        qemu_chr_add_handlers(chr, NULL, ivshmem_can_receive, 
> ivshmem_receive,
>                        ivshmem_event, s);
>      }
>  
> @@ -722,8 +722,8 @@ static int pci_ivshmem_init(PCIDevice *dev)
>  
>          s->eventfd_chr = g_malloc0(s->vectors * sizeof(CharDriverState *));
>  
> -        qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, 
> ivshmem_read,
> -                     ivshmem_event, s);
> +        qemu_chr_add_handlers(s->server_chr, NULL, ivshmem_can_receive,
> +                              ivshmem_read, ivshmem_event, s);
>      } else {
>          /* just map the file immediately, we're not using a server */
>          int fd;
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 81d901c..6035128 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -550,7 +550,7 @@ static void release_chr(Object *obj, const char *name, 
> void *opaque)
>      CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>  
>      if (*ptr) {
> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> +        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL, NULL);
>      }
>  }
>  
> diff --git a/hw/serial.c b/hw/serial.c
> index 60283ea..1cb8d0d 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -690,13 +690,13 @@ void serial_init_core(SerialState *s)
>  
>      qemu_register_reset(serial_reset, s);
>  
> -    qemu_chr_add_handlers(s->chr, serial_can_receive1, serial_receive1,
> +    qemu_chr_add_handlers(s->chr, NULL, serial_can_receive1, serial_receive1,
>                            serial_event, s);
>  }
>  
>  void serial_exit_core(SerialState *s)
>  {
> -    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL);
> +    qemu_chr_add_handlers(s->chr, NULL, NULL, NULL, NULL, NULL);
>      qemu_unregister_reset(serial_reset, s);
>  }
>  
> diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
> index 99b19df..5216b0c 100644
> --- a/hw/usb/dev-serial.c
> +++ b/hw/usb/dev-serial.c
> @@ -414,7 +414,7 @@ static void usb_serial_handle_destroy(USBDevice *dev)
>  {
>      USBSerialState *s = (USBSerialState *)dev;
>  
> -    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL);
> +    qemu_chr_add_handlers(s->cs, NULL, NULL, NULL, NULL, NULL);
>  }
>  
>  static int usb_serial_can_read(void *opaque)
> @@ -491,7 +491,7 @@ static int usb_serial_initfn(USBDevice *dev)
>          return -1;
>      }
>  
> -    qemu_chr_add_handlers(s->cs, usb_serial_can_read, usb_serial_read,
> +    qemu_chr_add_handlers(s->cs, NULL, usb_serial_can_read, usb_serial_read,
>                            usb_serial_event, s);
>      usb_serial_handle_reset(dev);
>  
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 0c95e6b..cf467eb 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -1082,7 +1082,7 @@ static int usbredir_initfn(USBDevice *udev)
>  
>      /* Let the backend know we are ready */
>      qemu_chr_fe_open(dev->cs);
> -    qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
> +    qemu_chr_add_handlers(dev->cs, NULL, usbredir_chardev_can_read,
>                            usbredir_chardev_read, usbredir_chardev_event, 
> dev);
>  
>      qemu_add_vm_change_state_handler(usbredir_vm_state_change, dev);
> diff --git a/hw/virtio-console.c b/hw/virtio-console.c
> index cffee3d..b6022b0 100644
> --- a/hw/virtio-console.c
> +++ b/hw/virtio-console.c
> @@ -74,6 +74,14 @@ static void guest_close(VirtIOSerialPort *port)
>      qemu_chr_fe_close(vcon->chr);
>  }
>  
> +/* Connectedness of the guest */
> +static int chr_connected(void *opaque)
> +{
> +    VirtConsole *vcon = opaque;
> +
> +    return virtio_serial_guest_connected(&vcon->port);
> +}
> +
>  /* Readiness of the guest to accept data on a port */
>  static int chr_can_read(void *opaque)
>  {
> @@ -117,8 +125,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
>      }
>  
>      if (vcon->chr) {
> -        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
> -                              vcon);
> +        qemu_chr_add_handlers(vcon->chr, chr_connected, chr_can_read, 
> chr_read,
> +                              chr_event, vcon);
>      }
>  
>      return 0;
> diff --git a/main-loop.h b/main-loop.h
> index 326c742..84f5750 100644
> --- a/main-loop.h
> +++ b/main-loop.h
> @@ -165,6 +165,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc 
> *func, void *opaque);
>  
>  typedef void IOReadHandler(void *opaque, const uint8_t *buf, int size);
>  typedef int IOCanReadHandler(void *opaque);
> +typedef int IOConnectedHandler(void *opaque);

bool?

>  
>  /**
>   * qemu_set_fd_handler2: Register a file descriptor with the main loop
> diff --git a/monitor.c b/monitor.c
> index c0e32d6..5a74a4e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4707,13 +4707,13 @@ void monitor_init(CharDriverState *chr, int flags)
>      if (monitor_ctrl_mode(mon)) {
>          mon->mc = g_malloc0(sizeof(MonitorControl));
>          /* Control mode requires special handlers */
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
> +        qemu_chr_add_handlers(chr, NULL, monitor_can_read, 
> monitor_control_read,
>                                monitor_control_event, mon);
>          qemu_chr_fe_set_echo(chr, true);
>  
>          json_message_parser_init(&mon->mc->parser, handle_qmp_command);
>      } else {
> -        qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
> +        qemu_chr_add_handlers(chr, NULL, monitor_can_read, monitor_read,
>                                monitor_event, mon);
>      }
>  
> diff --git a/net/slirp.c b/net/slirp.c
> index afb52c3..c963bee 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -659,7 +659,7 @@ static int slirp_guestfwd(SlirpState *s, const char 
> *config_str,
>          fwd->port = port;
>          fwd->slirp = s->slirp;
>  
> -        qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read,
> +        qemu_chr_add_handlers(fwd->hd, NULL, guestfwd_can_read, 
> guestfwd_read,
>                                NULL, fwd);
>      }
>      return 0;
> diff --git a/qemu-char.c b/qemu-char.c
> index 242b799..ba316e1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -152,6 +152,13 @@ int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void 
> *arg)
>      return s->chr_ioctl(s, cmd, arg);
>  }
>  
> +int qemu_chr_be_connected(CharDriverState *s)
> +{
> +    if (!s->chr_connected)
> +        return 1;
> +    return s->chr_connected(s->handler_opaque);
> +}
> +
>  int qemu_chr_be_can_write(CharDriverState *s)
>  {
>      if (!s->chr_can_read)
> @@ -194,6 +201,7 @@ void qemu_chr_fe_printf(CharDriverState *s, const char 
> *fmt, ...)
>  }
>  
>  void qemu_chr_add_handlers(CharDriverState *s,
> +                           IOConnectedHandler *fd_connected,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
>                             IOEventHandler *fd_event,
> @@ -203,6 +211,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
>          /* chr driver being released. */
>          ++s->avail_connections;
>      }
> +    s->chr_connected = fd_connected;
>      s->chr_can_read = fd_can_read;
>      s->chr_read = fd_read;
>      s->chr_event = fd_event;
> @@ -457,7 +466,7 @@ static void mux_chr_update_read_handler(CharDriverState 
> *chr)
>      d->chr_event[d->mux_cnt] = chr->chr_event;
>      /* Fix up the real driver with mux routines */
>      if (d->mux_cnt == 0) {
> -        qemu_chr_add_handlers(d->drv, mux_chr_can_read, mux_chr_read,
> +        qemu_chr_add_handlers(d->drv, NULL, mux_chr_can_read, mux_chr_read,
>                                mux_chr_event, chr);
>      }
>      if (d->focus != -1) {
> diff --git a/qemu-char.h b/qemu-char.h
> index a121e04..6e93d55 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -61,6 +61,7 @@ struct CharDriverState {
>      int (*get_msgfd)(struct CharDriverState *s);
>      int (*chr_add_client)(struct CharDriverState *chr, int fd);
>      IOEventHandler *chr_event;
> +    IOConnectedHandler *chr_connected;
>      IOCanReadHandler *chr_can_read;
>      IOReadHandler *chr_read;
>      void *handler_opaque;
> @@ -202,6 +203,17 @@ int qemu_chr_fe_get_msgfd(CharDriverState *s);
>  int qemu_chr_be_can_write(CharDriverState *s);
>  
>  /**
> + * @qemu_chr_be_connected:
> + *
> + * Determine if backend is connected.  Connected means that data will be
> + * eventually read by the backend, even if at this moment
> + * qemu_chr_be_can_write would return 0.

Actually, the connected backend may disconnect without reading any more
data, but I understand what you're trying to say here.  Anybody got a a
more carefully worded explanation?

> + *
> + * Returns: 1 if backend is connected, 0 otherwise.
> + */
> +int qemu_chr_be_connected(CharDriverState *s);
> +
> +/**
>   * @qemu_chr_be_write:
>   *
>   * Write data from the back end to the front end.  Before issuing this call,
> @@ -224,6 +236,7 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, 
> int len);
>  void qemu_chr_be_event(CharDriverState *s, int event);
>  
>  void qemu_chr_add_handlers(CharDriverState *s,
> +                           IOConnectedHandler *fd_connected,
>                             IOCanReadHandler *fd_can_read,
>                             IOReadHandler *fd_read,
>                             IOEventHandler *fd_event,
> diff --git a/qtest.c b/qtest.c
> index fbfab4e..c5f4193 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -425,7 +425,7 @@ int qtest_init(void)
>      configure_icount("0");
>      chr = qemu_chr_new("qtest", qtest_chrdev, NULL);
>  
> -    qemu_chr_add_handlers(chr, qtest_can_read, qtest_read, qtest_event, chr);
> +    qemu_chr_add_handlers(chr, NULL, qtest_can_read, qtest_read, 
> qtest_event, chr);
>      qemu_chr_fe_set_echo(chr, true);
>  
>      inbuf = g_string_new("");