Re: [Qemu-devel] [PATCH 13/38] ivshmem: Don't destroy the chardev on version mismatch

[Marc-André Lureau]

Hi

On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
> Yes, the chardev is commonly useless after we read a bad version from
> it, but destroying it is inappropriate anyway: the user created it, so
> the user should be able to hold on to it as long as he likes.  We
> don't destroy it on other errors.  Screwed up in commit 5105b1d.
>
> Stop reading instead.
>
> Also note QEMU's behavior in ivshmem-spec.txt.
>
> Signed-off-by: Markus Armbruster <address@hidden>

Interestingly enough, it seems both smartcard "passthru" and usb
redirect do chr_delete(). It would a nice follow up to standardize
this there too.

Reviewed-by: Marc-André Lureau <address@hidden>



> ---
>  docs/specs/ivshmem-spec.txt | 3 +++
>  hw/misc/ivshmem.c           | 3 +--
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
> index 0835ba1..4fc6f37 100644
> --- a/docs/specs/ivshmem-spec.txt
> +++ b/docs/specs/ivshmem-spec.txt
> @@ -188,6 +188,9 @@ Each message consists of a single 8 byte little-endian 
> signed number,
>  and may be accompanied by a file descriptor via SCM_RIGHTS.  Both
>  client and server close the connection on error.
>
> +Note: QEMU currently doesn't close the connection right on error, but
> +only when the character device is destroyed.
> +
>  On connect, the server sends the following messages in order:
>
>  1. The protocol version number, currently zero.  The client should
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 7119a07..2850e8a 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -707,8 +707,7 @@ static void ivshmem_check_version(void *opaque, const 
> uint8_t * buf, int size)
>      if (tmp != -1 || version != IVSHMEM_PROTOCOL_VERSION) {
>          fprintf(stderr, "incompatible version, you are connecting to a 
> ivshmem-"
>                  "server using a different protocol please check your 
> setup\n");
> -        qemu_chr_delete(s->server_chr);
> -        s->server_chr = NULL;
> +        qemu_chr_add_handlers(s->server_chr, NULL, NULL, NULL, s);
>          return;
>      }
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau