Re: [Qemu-devel] [PATCH v4 47/47] ivshmem: use little-endian int64_t for the protocol

[Claudio Fontana]

On 24.09.2015 13:37, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> The current ivshmem protocol uses 'long' for integers. But the
> sizeof(long) depends on the host and the endianess is not defined, which
> may cause portability troubles.
> 
> Instead, switch to using little-endian int64_t. This breaks the
> protocol, except on x64 little-endian host where this change
> should be compatible.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  contrib/ivshmem-client/ivshmem-client.c | 11 ++++++-----
>  contrib/ivshmem-client/ivshmem-client.h |  4 ++--
>  contrib/ivshmem-server/ivshmem-server.c |  5 +++--
>  contrib/ivshmem-server/ivshmem-server.h |  4 ++--
>  docs/specs/ivshmem_device_spec.txt      |  2 +-
>  hw/misc/ivshmem.c                       | 29 +++++++++++++++++++----------
>  6 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/contrib/ivshmem-client/ivshmem-client.c 
> b/contrib/ivshmem-client/ivshmem-client.c
> index a8477d8..4b5786c 100644
> --- a/contrib/ivshmem-client/ivshmem-client.c
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -24,7 +24,7 @@
>  
>  /* read message from the unix socket */
>  static int
> -ivshmem_client_read_one_msg(IvshmemClient *client, long *index, int *fd)
> +ivshmem_client_read_one_msg(IvshmemClient *client, int64_t *index, int *fd)
>  {
>      int ret;
>      struct msghdr msg;
> @@ -45,7 +45,7 @@ ivshmem_client_read_one_msg(IvshmemClient *client, long 
> *index, int *fd)
>      msg.msg_controllen = sizeof(msg_control);
>  
>      ret = recvmsg(client->sock_fd, &msg, 0);
> -    if (ret < 0) {
> +    if (ret < sizeof(*index)) {
>          IVSHMEM_CLIENT_DEBUG(client, "cannot read message: %s\n",
>                               strerror(errno));
>          return -1;
> @@ -55,6 +55,7 @@ ivshmem_client_read_one_msg(IvshmemClient *client, long 
> *index, int *fd)
>          return -1;
>      }
>  
> +    *index = GINT64_FROM_LE(*index);
>      *fd = -1;
>  
>      for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg = CMSG_NXTHDR(&msg, cmsg)) {
> @@ -91,7 +92,7 @@ static int
>  ivshmem_client_handle_server_msg(IvshmemClient *client)
>  {
>      IvshmemClientPeer *peer;
> -    long peer_id;
> +    int64_t peer_id;
>      int ret, fd;
>  
>      ret = ivshmem_client_read_one_msg(client, &peer_id, &fd);
> @@ -179,7 +180,7 @@ ivshmem_client_connect(IvshmemClient *client)
>  {
>      struct sockaddr_un sun;
>      int fd, ret;
> -    long tmp;
> +    int64_t tmp;
>  
>      IVSHMEM_CLIENT_DEBUG(client, "connect to client %s\n",
>                           client->unix_sock_path);
> @@ -401,7 +402,7 @@ ivshmem_client_notify_broadcast(const IvshmemClient 
> *client)
>  
>  /* lookup peer from its id */
>  IvshmemClientPeer *
> -ivshmem_client_search_peer(IvshmemClient *client, long peer_id)
> +ivshmem_client_search_peer(IvshmemClient *client, int64_t peer_id)
>  {
>      IvshmemClientPeer *peer;
>  
> diff --git a/contrib/ivshmem-client/ivshmem-client.h 
> b/contrib/ivshmem-client/ivshmem-client.h
> index 9215f34..3a4f809 100644
> --- a/contrib/ivshmem-client/ivshmem-client.h
> +++ b/contrib/ivshmem-client/ivshmem-client.h
> @@ -43,7 +43,7 @@
>   */
>  typedef struct IvshmemClientPeer {
>      QTAILQ_ENTRY(IvshmemClientPeer) next;    /**< next in list*/
> -    long id;                                 /**< the id of the peer */
> +    int64_t id;                              /**< the id of the peer */
>      int vectors[IVSHMEM_CLIENT_MAX_VECTORS]; /**< one fd per vector */
>      unsigned vectors_count;                  /**< number of vectors */
>  } IvshmemClientPeer;
> @@ -198,7 +198,7 @@ int ivshmem_client_notify_broadcast(const IvshmemClient 
> *client);
>   * Returns:  The peer structure, or NULL if not found
>   */
>  IvshmemClientPeer *
> -ivshmem_client_search_peer(IvshmemClient *client, long peer_id);
> +ivshmem_client_search_peer(IvshmemClient *client, int64_t peer_id);
>  
>  /**
>   * Dump information of this ivshmem client on stdout
> diff --git a/contrib/ivshmem-server/ivshmem-server.c 
> b/contrib/ivshmem-server/ivshmem-server.c
> index 060f414..3742a78 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -33,7 +33,7 @@
>  
>  /* send message to a client unix socket */
>  static int
> -ivshmem_server_send_one_msg(int sock_fd, long peer_id, int fd)
> +ivshmem_server_send_one_msg(int sock_fd, int64_t peer_id, int fd)
>  {
>      int ret;
>      struct msghdr msg;
> @@ -44,6 +44,7 @@ ivshmem_server_send_one_msg(int sock_fd, long peer_id, int 
> fd)
>      } msg_control;
>      struct cmsghdr *cmsg;
>  
> +    peer_id = GINT64_TO_LE(peer_id);
>      iov[0].iov_base = &peer_id;
>      iov[0].iov_len = sizeof(peer_id);
>  
> @@ -448,7 +449,7 @@ ivshmem_server_handle_fds(IvshmemServer *server, fd_set 
> *fds, int maxfd)
>  
>  /* lookup peer from its id */
>  IvshmemServerPeer *
> -ivshmem_server_search_peer(IvshmemServer *server, long peer_id)
> +ivshmem_server_search_peer(IvshmemServer *server, int64_t peer_id)
>  {
>      IvshmemServerPeer *peer;
>  
> diff --git a/contrib/ivshmem-server/ivshmem-server.h 
> b/contrib/ivshmem-server/ivshmem-server.h
> index 65b3c2d..d179f22 100644
> --- a/contrib/ivshmem-server/ivshmem-server.h
> +++ b/contrib/ivshmem-server/ivshmem-server.h
> @@ -50,7 +50,7 @@
>  typedef struct IvshmemServerPeer {
>      QTAILQ_ENTRY(IvshmemServerPeer) next;    /**< next in list*/
>      int sock_fd;                             /**< connected unix sock */
> -    long id;                                 /**< the id of the peer */
> +    int64_t id;                              /**< the id of the peer */
>      int vectors[IVSHMEM_SERVER_MAX_VECTORS]; /**< one fd per vector */
>      unsigned vectors_count;                  /**< number of vectors */
>  } IvshmemServerPeer;
> @@ -154,7 +154,7 @@ int ivshmem_server_handle_fds(IvshmemServer *server, 
> fd_set *fds, int maxfd);
>   * Returns:  The peer structure, or NULL if not found
>   */
>  IvshmemServerPeer *
> -ivshmem_server_search_peer(IvshmemServer *server, long peer_id);
> +ivshmem_server_search_peer(IvshmemServer *server, int64_t peer_id);
>  
>  /**
>   * Dump information of this ivshmem server and its peers on stdout
> diff --git a/docs/specs/ivshmem_device_spec.txt 
> b/docs/specs/ivshmem_device_spec.txt
> index 3435116..d318d65 100644
> --- a/docs/specs/ivshmem_device_spec.txt
> +++ b/docs/specs/ivshmem_device_spec.txt
> @@ -61,7 +61,7 @@ This server code is available in 
> qemu.git/contrib/ivshmem-server.
>  
>  The server must be started on the host before any guest.
>  It creates a shared memory object then waits for clients to connect on a unix
> -socket.
> +socket. All the messages are little-endian int64_t integer.
>  
>  For each client (QEMU process) that connects to the server:
>  - the server sends a protocol version, if client does not support it, the 
> client
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 39c0791..71e58b8 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -276,7 +276,7 @@ static void ivshmem_receive(void *opaque, const uint8_t 
> *buf, int size)
>  
>  static int ivshmem_can_receive(void * opaque)
>  {
> -    return sizeof(long);
> +    return sizeof(int64_t);
>  }
>  
>  static void ivshmem_event(void *opaque, int event)
> @@ -516,7 +516,7 @@ static bool fifo_update_and_get(IVShmemState *s, const 
> uint8_t *buf, int size,
>      const uint8_t *p;
>      uint32_t num;
>  
> -    assert(len <= sizeof(long)); /* limitation of the fifo */
> +    assert(len <= sizeof(int64_t)); /* limitation of the fifo */
>      if (fifo8_is_empty(&s->incoming_fifo) && size == len) {
>          memcpy(data, buf, size);
>          return true;
> @@ -524,7 +524,7 @@ static bool fifo_update_and_get(IVShmemState *s, const 
> uint8_t *buf, int size,
>  
>      IVSHMEM_DPRINTF("short read of %d bytes\n", size);
>  
> -    num = MIN(size, sizeof(long) - fifo8_num_used(&s->incoming_fifo));
> +    num = MIN(size, sizeof(int64_t) - fifo8_num_used(&s->incoming_fifo));
>      fifo8_push_all(&s->incoming_fifo, buf, num);
>  
>      if (fifo8_num_used(&s->incoming_fifo) < len) {
> @@ -546,6 +546,17 @@ static bool fifo_update_and_get(IVShmemState *s, const 
> uint8_t *buf, int size,
>      return true;
>  }
>  
> +static bool fifo_update_and_get_i64(IVShmemState *s,
> +                                    const uint8_t *buf, int size, int64_t 
> *i64)
> +{
> +    if (fifo_update_and_get(s, buf, size, i64, sizeof(*i64))) {
> +        *i64 = GINT64_FROM_LE(*i64);
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static int ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector)
>  {
>      PCIDevice *pdev = PCI_DEVICE(s);
> @@ -598,12 +609,11 @@ static void ivshmem_read(void *opaque, const uint8_t 
> *buf, int size)
>      IVShmemState *s = opaque;
>      int incoming_fd;
>      int new_eventfd;
> -    long incoming_posn;
> +    int64_t incoming_posn;
>      Error *err = NULL;
>      Peer *peer;
>  
> -    if (!fifo_update_and_get(s, buf, size,
> -                             &incoming_posn, sizeof(incoming_posn))) {
> +    if (!fifo_update_and_get_i64(s, buf, size, &incoming_posn)) {
>          return;
>      }
>  
> @@ -711,10 +721,9 @@ static void ivshmem_check_version(void *opaque, const 
> uint8_t * buf, int size)
>  {
>      IVShmemState *s = opaque;
>      int tmp;
> -    long version;
> +    int64_t version;
>  
> -    if (!fifo_update_and_get(s, buf, size,
> -                             &version, sizeof(version))) {
> +    if (!fifo_update_and_get_i64(s, buf, size, &version)) {
>          return;
>      }
>  
> @@ -869,7 +878,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error 
> **errp)
>          s->ivshmem_size = size;
>      }
>  
> -    fifo8_create(&s->incoming_fifo, sizeof(long));
> +    fifo8_create(&s->incoming_fifo, sizeof(int64_t));
>  
>      /* IRQFD requires MSI */
>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD) &&
> 

Reviewed-by: Claudio Fontana <address@hidden>