Re: [Qemu-devel] [PATCH 02/10] char/spice: discard write() if backend is disconnected

[Marc-André Lureau]

Hi

On Fri, Aug 3, 2018 at 7:36 PM, Marc-André Lureau
<address@hidden> wrote:
> Most chardev backend handle write() as discarded data if underlying
> system is disconnected. For unknown historical reasons, the Spice
> backend has "reliable" write. It will wait until the client end is
> reconnected to accept further write().
>
> Let's review Spice chardev usage and handling of a disconnected
> client:
>
>  * spice vdagent
>    The agent will reopen the virtio port on disconnect.
>
>  * usb redirection
>    A disconnect creates a device disconnection.
>
>  * smartcard emulation
>    Data is discarded in passthru_apdu_from_guest()

Doing more testing, I realize this creates is a regression on Spice
smartcard. Sadly, the Spice smartcard channel isn't actually based on
spicevmc (as the chardev name may imply), and doesn't set the
connected sif->state(). I am sending a patch to spice-devel to fix
this. I'll update this patch to set the chardev opened at "smartcard"
spicevmc creation time when we have an old spice server.

>
>  * spice webdavd
>    The daemon will restart the service, and reopen the virtio port.
>
>  * spice ports (serial console, qemu monitor..)
>    Depends on the associated device or usage.
>
>    - 16550A serial does nothing special, and may block guest on write
>
>    - QMP/HMP monitor have some CLOSED event handling, but want to
>      flush the write, which will finish when a new client connects.
>
> For all these use cases, it is better to discard pending write when
> the client is disconnected, and expect the device/agent to behave
> correctly on CHR_EVENT_CLOSED (to stop reading and writing from
> chardev).
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  chardev/spice.c      | 6 ++++++
>  chardev/trace-events | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/chardev/spice.c b/chardev/spice.c
> index fe06034d7f..6ad95ffe62 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -212,6 +212,12 @@ static int spice_chr_write(Chardev *chr, const uint8_t 
> *buf, int len)
>      int read_bytes;
>
>      assert(s->datalen == 0);
> +
> +    if (!chr->be_open) {
> +        trace_spice_chr_discard_write(len);
> +        return len;
> +    }
> +
>      s->datapos = buf;
>      s->datalen = len;
>      spice_server_char_device_wakeup(&s->sin);
> diff --git a/chardev/trace-events b/chardev/trace-events
> index d0e5f3bbc1..b8a7596344 100644
> --- a/chardev/trace-events
> +++ b/chardev/trace-events
> @@ -10,6 +10,7 @@ wct_cmd_other(const char *cmd) "%s"
>  wct_speed(int speed) "%d"
>
>  # chardev/spice.c
> +spice_chr_discard_write(int len) "spice chr write discarded %d"
>  spice_vmc_write(ssize_t out, int len) "spice wrote %zd of requested %d"
>  spice_vmc_read(int bytes, int len) "spice read %d of requested %d"
>  spice_vmc_register_interface(void *scd) "spice vmc registered interface %p"
> --
> 2.18.0.547.g1d89318c48
>
>



-- 
Marc-André Lureau