Re: [Qemu-devel] [PATCH v6 3/3] ivshmem: add check on protocol version in QEMU

[Michael S. Tsirkin]

On Mon, Sep 08, 2014 at 11:17:50AM +0200, David Marchand wrote:
> Send a protocol version as the first message from server, clients must close
> communication if they don't support this protocol version.

What's the motivation here?
This is at best a way to break all clients if an incompatible
change in the server is made.
Would not it be better to send a bitmap, or a list of supported
versions, so it's possible to write servers compatible
with multiple clients?

> Older QEMUs should be fine with this change in the protocol since they 
> overrides

they override

> their own vm_id on reception of an id associated to no eventfd.

associated with?


I don't understand how compatibility works exactly,
a bit more explanation in code comments would be nice.
In particular new client won't work with old server,
will it?

> 
> Signed-off-by: David Marchand <address@hidden>
> ---
>  contrib/ivshmem-client/ivshmem-client.c |   13 ++++++++++---
>  contrib/ivshmem-client/ivshmem-client.h |    1 +
>  contrib/ivshmem-server/ivshmem-server.c |    9 +++++++++
>  contrib/ivshmem-server/ivshmem-server.h |    1 +
>  docs/specs/ivshmem_device_spec.txt      |    9 ++++++---
>  hw/misc/ivshmem.c                       |   27 +++++++++++++++++++++++++--
>  include/hw/misc/ivshmem.h               |   17 +++++++++++++++++
>  7 files changed, 69 insertions(+), 8 deletions(-)
>  create mode 100644 include/hw/misc/ivshmem.h
> 
> diff --git a/contrib/ivshmem-client/ivshmem-client.c 
> b/contrib/ivshmem-client/ivshmem-client.c
> index 11c805c..4413e26 100644
> --- a/contrib/ivshmem-client/ivshmem-client.c
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -201,10 +201,17 @@ ivshmem_client_connect(IvshmemClient *client)
>          goto err_close;
>      }
>  
> -    /* first, we expect our index + a fd == -1 */
> +    /* first, we expect a protocol version */
> +    if (ivshmem_client_read_one_msg(client, &tmp, &fd) < 0 ||
> +        (tmp != IVSHMEM_PROTOCOL_VERSION) || fd != -1) {
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n");
> +        goto err_close;
> +    }
> +
> +    /* then, we expect our index + a fd == -1 */
>      if (ivshmem_client_read_one_msg(client, &client->local.id, &fd) < 0 ||
>          client->local.id < 0 || fd != -1) {
> -        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server\n");
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n");
>          goto err_close;
>      }
>      IVSHMEM_CLIENT_DEBUG(client, "our_id=%ld\n", client->local.id);
> @@ -216,7 +223,7 @@ ivshmem_client_connect(IvshmemClient *client)
>          if (fd >= 0) {
>              close(fd);
>          }
> -        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (2)\n");
> +        IVSHMEM_CLIENT_DEBUG(client, "cannot read from server (3)\n");
>          goto err_close;
>      }
>      client->shm_fd = fd;
> diff --git a/contrib/ivshmem-client/ivshmem-client.h 
> b/contrib/ivshmem-client/ivshmem-client.h
> index 284c4a3..9215f34 100644
> --- a/contrib/ivshmem-client/ivshmem-client.h
> +++ b/contrib/ivshmem-client/ivshmem-client.h
> @@ -23,6 +23,7 @@
>  #include <sys/select.h>
>  
>  #include "qemu/queue.h"
> +#include "hw/misc/ivshmem.h"
>  
>  /**
>   * Maximum number of notification vectors supported by the client
> diff --git a/contrib/ivshmem-server/ivshmem-server.c 
> b/contrib/ivshmem-server/ivshmem-server.c
> index 91dc208..ea290f6 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -99,6 +99,15 @@ ivshmem_server_send_initial_info(IvshmemServer *server, 
> IvshmemServerPeer *peer)
>  {
>      int ret;
>  
> +    /* send our protocol version first */
> +    ret = ivshmem_server_send_one_msg(peer->sock_fd, 
> IVSHMEM_PROTOCOL_VERSION,
> +                                      -1);
> +    if (ret < 0) {
> +        IVSHMEM_SERVER_DEBUG(server, "cannot send version: %s\n",
> +                             strerror(errno));
> +        return -1;
> +    }
> +
>      /* send the peer id to the client */
>      ret = ivshmem_server_send_one_msg(peer->sock_fd, peer->id, -1);
>      if (ret < 0) {
> diff --git a/contrib/ivshmem-server/ivshmem-server.h 
> b/contrib/ivshmem-server/ivshmem-server.h
> index f85dcd2..38df7bb 100644
> --- a/contrib/ivshmem-server/ivshmem-server.h
> +++ b/contrib/ivshmem-server/ivshmem-server.h
> @@ -30,6 +30,7 @@
>  #include <sys/select.h>
>  
>  #include "qemu/queue.h"
> +#include "hw/misc/ivshmem.h"
>  
>  /**
>   * Maximum number of notification vectors supported by the server
> diff --git a/docs/specs/ivshmem_device_spec.txt 
> b/docs/specs/ivshmem_device_spec.txt
> index 12f338e..3435116 100644
> --- a/docs/specs/ivshmem_device_spec.txt
> +++ b/docs/specs/ivshmem_device_spec.txt
> @@ -64,6 +64,8 @@ It creates a shared memory object then waits for clients to 
> connect on a unix
>  socket.
>  
>  For each client (QEMU process) that connects to the server:
> +- the server sends a protocol version, if client does not support it, the 
> client
> +  closes the communication,
>  - the server assigns an ID for this client and sends this ID to him as the 
> first
>    message,
>  - the server sends a fd to the shared memory object to this client,
> @@ -86,9 +88,10 @@ been provided in qemu.git/contrib/ivshmem-client for debug.
>  
>  *QEMU as an ivshmem client*
>  
> -At initialisation, when creating the ivshmem device, QEMU gets its ID from 
> the
> -server then makes it available through BAR0 IVPosition register for the VM to
> -use (see 'PCI device registers' subsection).
> +At initialisation, when creating the ivshmem device, QEMU first receives a
> +protocol version and closes communication with server if it does not match.
> +Then, QEMU gets its ID from the server then makes it available through BAR0
> +IVPosition register for the VM to use (see 'PCI device registers' 
> subsection).
>  QEMU then uses the fd to the shared memory to map it to BAR2.
>  eventfds for all other clients received from the server are stored to 
> implement
>  BAR0 Doorbell register (see 'PCI device registers' subsection).
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index bd9d718..e08632b 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -26,6 +26,8 @@
>  #include "qemu/event_notifier.h"
>  #include "sysemu/char.h"
>  
> +#include "hw/misc/ivshmem.h"
> +
>  #include <sys/mman.h>
>  #include <sys/types.h>
>  
> @@ -528,6 +530,27 @@ static void ivshmem_read(void *opaque, const uint8_t * 
> buf, int flags)
>      }
>  }
>  
> +static void ivshmem_check_version(void *opaque, const uint8_t * buf, int 
> flags)
> +{
> +    IVShmemState *s = opaque;
> +    int tmp;
> +    long version;
> +
> +    memcpy(&version, buf, sizeof(long));
> +    tmp = qemu_chr_fe_get_msgfd(s->server_chr);
> +    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;
> +        return;
> +    }
> +
> +    IVSHMEM_DPRINTF("version check ok, switch to real chardev handler\n");
> +    qemu_chr_add_handlers(s->server_chr, ivshmem_can_receive, ivshmem_read,
> +                          ivshmem_event, s);
> +}
> +
>  /* Select the MSI-X vectors used by device.
>   * ivshmem maps events to vectors statically, so
>   * we just enable all vectors on init and after reset. */
> @@ -742,8 +765,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, ivshmem_can_receive,
> +                              ivshmem_check_version, ivshmem_event, s);
>      } else {
>          /* just map the file immediately, we're not using a server */
>          int fd;
> diff --git a/include/hw/misc/ivshmem.h b/include/hw/misc/ivshmem.h
> new file mode 100644
> index 0000000..22c9f1f
> --- /dev/null
> +++ b/include/hw/misc/ivshmem.h
> @@ -0,0 +1,17 @@
> +/*
> + * Copyright 6WIND S.A., 2014
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#ifndef _IVSHMEM_H_
> +#define _IVSHMEM_H_
> +
> +/**
> + * Protocol version
> + */
> +#define IVSHMEM_PROTOCOL_VERSION 0x0
> +
> +#endif
> -- 
> 1.7.10.4