Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server

[Gonglei]

Hi,

> Subject: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
> 
> When using ivshmem devices, notifications between guests can be sent as
> interrupts using a ivshmem-server (typical use described in documentation).
> The client is provided as a debug tool.
> 
> Signed-off-by: Olivier Matz <address@hidden>
> Signed-off-by: David Marchand <address@hidden>
> ---
>  contrib/ivshmem-client/Makefile         |   29 +++
>  contrib/ivshmem-client/ivshmem-client.c |  418
> ++++++++++++++++++++++++++++++
>  contrib/ivshmem-client/ivshmem-client.h |  238 ++++++++++++++++++
>  contrib/ivshmem-client/main.c           |  246 ++++++++++++++++++
>  contrib/ivshmem-server/Makefile         |   29 +++
>  contrib/ivshmem-server/ivshmem-server.c |  420
> +++++++++++++++++++++++++++++++
>  contrib/ivshmem-server/ivshmem-server.h |  185 ++++++++++++++
>  contrib/ivshmem-server/main.c           |  296
> ++++++++++++++++++++++
>  qemu-doc.texi                           |   10 +-
>  9 files changed, 1868 insertions(+), 3 deletions(-)
>  create mode 100644 contrib/ivshmem-client/Makefile
>  create mode 100644 contrib/ivshmem-client/ivshmem-client.c
>  create mode 100644 contrib/ivshmem-client/ivshmem-client.h
>  create mode 100644 contrib/ivshmem-client/main.c
>  create mode 100644 contrib/ivshmem-server/Makefile
>  create mode 100644 contrib/ivshmem-server/ivshmem-server.c
>  create mode 100644 contrib/ivshmem-server/ivshmem-server.h
>  create mode 100644 contrib/ivshmem-server/main.c
> 
> diff --git a/contrib/ivshmem-client/Makefile b/contrib/ivshmem-client/Makefile
> new file mode 100644
> index 0000000..eee97c6
> --- /dev/null
> +++ b/contrib/ivshmem-client/Makefile
> @@ -0,0 +1,29 @@
> +# 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.
> +
> +S ?= $(CURDIR)
> +O ?= $(CURDIR)
> +
> +CFLAGS += -Wall -Wextra -Werror -g
> +LDFLAGS +=
> +LDLIBS += -lrt
> +
> +VPATH = $(S)
> +PROG = ivshmem-client
> +OBJS := $(O)/ivshmem-client.o
> +OBJS += $(O)/main.o
> +
> +$(O)/%.o: %.c
> +     $(CC) $(CFLAGS) -o $@ -c $<
> +
> +$(O)/$(PROG): $(OBJS)
> +     $(CC) $(LDFLAGS) -o $@ $^ $(LDLIBS)
> +
> +.PHONY: all
> +all: $(O)/$(PROG)
> +
> +clean:
> +     rm -f $(OBJS) $(O)/$(PROG)
> diff --git a/contrib/ivshmem-client/ivshmem-client.c
> b/contrib/ivshmem-client/ivshmem-client.c
> new file mode 100644
> index 0000000..2166b64
> --- /dev/null
> +++ b/contrib/ivshmem-client/ivshmem-client.c
> @@ -0,0 +1,418 @@
> +/*
> + * 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.
> + */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <inttypes.h>
> +#include <sys/queue.h>
> +
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +
> +#include "ivshmem-client.h"
> +
> +/* log a message on stdout if verbose=1 */
> +#define debug_log(client, fmt, ...) do { \
> +        if ((client)->verbose) {         \
> +            printf(fmt, ## __VA_ARGS__); \
> +        }                                \
> +    } while (0)
> +
> +/* read message from the unix socket */
> +static int
> +read_one_msg(struct ivshmem_client *client, long *index, int *fd)
> +{
> +    int ret;
> +    struct msghdr msg;
> +    struct iovec iov[1];
> +    union {
> +        struct cmsghdr cmsg;
> +        char control[CMSG_SPACE(sizeof(int))];
> +    } msg_control;
> +    struct cmsghdr *cmsg;
> +
> +    iov[0].iov_base = index;
> +    iov[0].iov_len = sizeof(*index);
> +
> +    memset(&msg, 0, sizeof(msg));
> +    msg.msg_iov = iov;
> +    msg.msg_iovlen = 1;
> +    msg.msg_control = &msg_control;
> +    msg.msg_controllen = sizeof(msg_control);
> +
> +    ret = recvmsg(client->sock_fd, &msg, 0);
> +    if (ret < 0) {
> +        debug_log(client, "cannot read message: %s\n", strerror(errno));
> +        return -1;
> +    }
> +    if (ret == 0) {
> +        debug_log(client, "lost connection to server\n");
> +        return -1;
> +    }
> +
> +    *fd = -1;
> +
> +    for (cmsg = CMSG_FIRSTHDR(&msg); cmsg; cmsg =
> CMSG_NXTHDR(&msg, cmsg)) {
> +
> +        if (cmsg->cmsg_len != CMSG_LEN(sizeof(int)) ||
> +            cmsg->cmsg_level != SOL_SOCKET ||
> +            cmsg->cmsg_type != SCM_RIGHTS) {
> +            continue;
> +        }
> +
> +        memcpy(fd, CMSG_DATA(cmsg), sizeof(*fd));
> +    }
> +
> +    return 0;
> +}
> +
> +/* free a peer when the server advertise a disconnection or when the
> + * client is freed */
> +static void
> +free_peer(struct ivshmem_client *client, struct ivshmem_client_peer *peer)
> +{
> +    unsigned vector;
> +
> +    TAILQ_REMOVE(&client->peer_list, peer, next);
> +    for (vector = 0; vector < peer->vectors_count; vector++) {
> +        close(peer->vectors[vector]);
> +    }
> +
> +    free(peer);
> +}
> +
> +/* handle message coming from server (new peer, new vectors) */
> +static int
> +handle_server_msg(struct ivshmem_client *client)
> +{
> +    struct ivshmem_client_peer *peer;
> +    long peer_id;
> +    int ret, fd;
> +
> +    ret = read_one_msg(client, &peer_id, &fd);
> +    if (ret < 0) {
> +        return -1;
> +    }
> +
> +    /* can return a peer or the local client */
> +    peer = ivshmem_client_search_peer(client, peer_id);
> +
> +    /* delete peer */
> +    if (fd == -1) {
> +
Maybe the above check should be moved before getting the peer.
And the next check peer is extra.

> +        if (peer == NULL || peer == &client->local) {
> +            debug_log(client, "receive delete for invalid peer %ld",

Missing '\n' ?

> peer_id);
> +            return -1;
> +        }
> +
> +        debug_log(client, "delete peer id = %ld\n", peer_id);
> +        free_peer(client, peer);
> +        return 0;
> +    }
> +
> +    /* new peer */
> +    if (peer == NULL) {
> +        peer = malloc(sizeof(*peer));

g_malloc0 ?.

> +        if (peer == NULL) {
> +            debug_log(client, "cannot allocate new peer\n");
> +            return -1;
> +        }
> +        memset(peer, 0, sizeof(*peer));
> +        peer->id = peer_id;
> +        peer->vectors_count = 0;
> +        TAILQ_INSERT_TAIL(&client->peer_list, peer, next);
> +        debug_log(client, "new peer id = %ld\n", peer_id);
> +    }
> +
> +    /* new vector */
> +    debug_log(client, "  new vector %d (fd=%d) for peer id %ld\n",
> +              peer->vectors_count, fd, peer->id);
> +    peer->vectors[peer->vectors_count] = fd;
> +    peer->vectors_count++;
> +
> +    return 0;
> +}
> +
> +/* init a new ivshmem client */
> +int
> +ivshmem_client_init(struct ivshmem_client *client, const char
> *unix_sock_path,
> +                    ivshmem_client_notif_cb_t notif_cb, void *notif_arg,
> +                    int verbose)
> +{
> +    unsigned i;
> +
> +    memset(client, 0, sizeof(*client));
> +
> +    snprintf(client->unix_sock_path, sizeof(client->unix_sock_path),
> +             "%s", unix_sock_path);
> +
> +    for (i = 0; i < IVSHMEM_CLIENT_MAX_VECTORS; i++) {
> +        client->local.vectors[i] = -1;
> +    }
> +
> +    TAILQ_INIT(&client->peer_list);
> +    client->local.id = -1;
> +
> +    client->notif_cb = notif_cb;
> +    client->notif_arg = notif_arg;
> +    client->verbose = verbose;

Missing client->sock_fd = -1; ?
> +
> +    return 0;
> +}
> +
> +/* create and connect to the unix socket */
> +int
> +ivshmem_client_connect(struct ivshmem_client *client)
> +{
> +    struct sockaddr_un sun;
> +    int fd;
> +    long tmp;
> +
> +    debug_log(client, "connect to client %s\n", client->unix_sock_path);
> +
> +    client->sock_fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +    if (client->sock_fd < 0) {
> +        debug_log(client, "cannot create socket: %s\n", strerror(errno));
> +        return -1;
> +    }
> +
> +    sun.sun_family = AF_UNIX;
> +    snprintf(sun.sun_path, sizeof(sun.sun_path), "%s",
> client->unix_sock_path);
> +    if (connect(client->sock_fd, (struct sockaddr *)&sun, sizeof(sun)) < 0) {
> +        debug_log(client, "cannot connect to %s: %s\n", sun.sun_path,
> +                  strerror(errno));
> +        close(client->sock_fd);
> +        client->sock_fd = -1;
> +        return -1;
> +    }
> +
> +    /* first, we expect our index + a fd == -1 */
> +    if (read_one_msg(client, &client->local.id, &fd) < 0 ||
> +        client->local.id < 0 || fd != -1) {
Why not fd < 0 ?

> +        debug_log(client, "cannot read from server\n");
> +        close(client->sock_fd);
> +        client->sock_fd = -1;
> +        return -1;
> +    }
> +    debug_log(client, "our_id=%ld\n", client->local.id);
> +
> +    /* now, we expect shared mem fd + a -1 index, note that shm fd
> +     * is not used */
> +    if (read_one_msg(client, &tmp, &fd) < 0 ||
> +        tmp != -1 || fd < 0) {
> +        debug_log(client, "cannot read from server (2)\n");
> +        close(client->sock_fd);
> +        client->sock_fd = -1;
> +        return -1;
I think the error logic handle can move the end of this function, reducing 
duplicated code. Something like this:

        goto err;
  }
err:
        debug_log(client, "cannot read from server (2)\n");
    close(client->sock_fd);
    client->sock_fd = -1;
    return -1;

> +    }
> +    debug_log(client, "shm_fd=%d\n", fd);
> +
> +    return 0;
> +}
> +
> +/* close connection to the server, and free all peer structures */
> +void
> +ivshmem_client_close(struct ivshmem_client *client)
> +{
> +    struct ivshmem_client_peer *peer;
> +    unsigned i;
> +
> +    debug_log(client, "close client\n");
> +
> +    while ((peer = TAILQ_FIRST(&client->peer_list)) != NULL) {
> +        free_peer(client, peer);
> +    }
> +
> +    close(client->sock_fd);
> +    client->sock_fd = -1;
> +    client->local.id = -1;
> +    for (i = 0; i < IVSHMEM_CLIENT_MAX_VECTORS; i++) {
> +        client->local.vectors[i] = -1;
> +    }
> +}
> +
> +/* get the fd_set according to the unix socket and peer list */
> +void
> +ivshmem_client_get_fds(const struct ivshmem_client *client, fd_set *fds,
> +                       int *maxfd)
> +{
> +    int fd;
> +    unsigned vector;
> +
> +    FD_SET(client->sock_fd, fds);
> +    if (client->sock_fd >= *maxfd) {
> +        *maxfd = client->sock_fd + 1;
> +    }
> +
> +    for (vector = 0; vector < client->local.vectors_count; vector++) {
> +        fd = client->local.vectors[vector];
> +        FD_SET(fd, fds);
> +        if (fd >= *maxfd) {
> +            *maxfd = fd + 1;
> +        }
> +    }
> +}
> +
> +/* handle events from eventfd: just print a message on notification */
> +static int
> +handle_event(struct ivshmem_client *client, const fd_set *cur, int maxfd)
> +{
> +    struct ivshmem_client_peer *peer;
> +    uint64_t kick;
> +    unsigned i;
> +    int ret;
> +
> +    peer = &client->local;
> +
> +    for (i = 0; i < peer->vectors_count; i++) {
> +        if (peer->vectors[i] >= maxfd || !FD_ISSET(peer->vectors[i], cur)) {
> +            continue;
> +        }
> +
> +        ret = read(peer->vectors[i], &kick, sizeof(kick));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        if (ret != sizeof(kick)) {
> +            debug_log(client, "invalid read size = %d\n", ret);
> +            errno = EINVAL;
> +            return -1;
> +        }
> +        debug_log(client, "received event on fd %d vector %d: %ld\n",
> +                  peer->vectors[i], i, kick);
> +        if (client->notif_cb != NULL) {
> +            client->notif_cb(client, peer, i, client->notif_arg);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/* read and handle new messages on the given fd_set */
> +int
> +ivshmem_client_handle_fds(struct ivshmem_client *client, fd_set *fds, int
> maxfd)
> +{
> +    if (client->sock_fd < maxfd && FD_ISSET(client->sock_fd, fds) &&
> +        handle_server_msg(client) < 0 && errno != EINTR) {
> +        debug_log(client, "handle_server_msg() failed\n");
> +        return -1;
> +    } else if (handle_event(client, fds, maxfd) < 0 && errno != EINTR) {
> +        debug_log(client, "handle_event() failed\n");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +/* send a notification on a vector of a peer */
> +int
> +ivshmem_client_notify(const struct ivshmem_client *client,
> +                      const struct ivshmem_client_peer *peer, unsigned
> vector)
> +{
> +    uint64_t kick;
> +    int fd;
> +
> +    if (vector > peer->vectors_count) {

Maybe if (vector >= peer->vectors_count) , otherwise the
peer->vectors[] array bounds.

> +        debug_log(client, "invalid vector %u on peer %ld\n", vector,
> peer->id);
> +        return -1;
> +    }
> +    fd = peer->vectors[vector];

Or fd = peer->vectors[vector - 1]; ?

> +    debug_log(client, "notify peer %ld on vector %d, fd %d\n", peer->id,
> vector,
> +              fd);
> +
> +    kick = 1;
> +    if (write(fd, &kick, sizeof(kick)) != sizeof(kick)) {
> +        fprintf(stderr, "could not write to %d: %s\n", peer->vectors[vector],
> +                strerror(errno));
Why not debug_log() at this here?

> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +/* send a notification to all vectors of a peer */
> +int
> +ivshmem_client_notify_all_vects(const struct ivshmem_client *client,
> +                                const struct ivshmem_client_peer
> *peer)
> +{
> +    unsigned vector;
> +    int ret = 0;
> +
> +    for (vector = 0; vector < peer->vectors_count; vector++) {
> +        if (ivshmem_client_notify(client, peer, vector) < 0) {
> +            ret = -1;
The ret's value will be covered when multi clients failed. Do we need
store the failed status for server?.

> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +/* send a notification to all peers */
> +int
> +ivshmem_client_notify_broadcast(const struct ivshmem_client *client)
> +{
> +    struct ivshmem_client_peer *peer;
> +    int ret = 0;
> +
> +    TAILQ_FOREACH(peer, &client->peer_list, next) {
> +        if (ivshmem_client_notify_all_vects(client, peer) < 0) {
> +            ret = -1;
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +/* lookup peer from its id */
> +struct ivshmem_client_peer *
> +ivshmem_client_search_peer(struct ivshmem_client *client, long peer_id)
> +{
> +    struct ivshmem_client_peer *peer;
> +
> +    if (peer_id == client->local.id) {
> +        return &client->local;
> +    }
> +
> +    TAILQ_FOREACH(peer, &client->peer_list, next) {
> +        if (peer->id == peer_id) {
> +            return peer;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +/* dump our info, the list of peers their vectors on stdout */
> +void
> +ivshmem_client_dump(const struct ivshmem_client *client)
> +{
> +    const struct ivshmem_client_peer *peer;
> +    unsigned vector;
> +
> +    /* dump local infos */
> +    peer = &client->local;
> +    printf("our_id = %ld\n", peer->id);
> +    for (vector = 0; vector < peer->vectors_count; vector++) {
> +        printf("  vector %d is enabled (fd=%d)\n", vector,
> +               peer->vectors[vector]);
> +    }
> +
> +    /* dump peers */
> +    TAILQ_FOREACH(peer, &client->peer_list, next) {
> +        printf("peer_id = %ld\n", peer->id);
> +
> +        for (vector = 0; vector < peer->vectors_count; vector++) {
> +            printf("  vector %d is enabled (fd=%d)\n", vector,
> +                   peer->vectors[vector]);
> +        }
> +    }
> +}

To be continued...

Best regards,
-Gonglei