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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4157 bytes --]

On Fri, Aug 08, 2014 at 10:55:17AM +0200, David Marchand wrote:

Looks good, a few minor comments:

> 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)

CCed Peter Maydell for a second opinion, I'd suggest hooking up to
QEMU's top-level ./Makefile.  QEMU does not do recursive make.

The advantages of hooking up QEMU's Makefile are:

1. So that ivshmem client/server code is built by default (on supported
   host platforms) and bitrot is avoided.

2. So that you don't have to duplicate rules.mak or any other build
   infrastructure.

> +/**
> + * Structure storing a peer
> + *
> + * Each time a client connects to an ivshmem server, it is advertised to
> + * all connected clients through the unix socket. When our ivshmem
> + * client receives a notification, it creates a ivshmem_client_peer
> + * structure to store the infos of this peer.
> + *
> + * This structure is also used to store the information of our own
> + * client in (struct ivshmem_client)->local.
> + */
> +struct ivshmem_client_peer {
> +    TAILQ_ENTRY(ivshmem_client_peer) next;    /**< next in list*/
> +    long id;                                    /**< the id of the peer */
> +    int vectors[IVSHMEM_CLIENT_MAX_VECTORS];  /**< one fd per vector */
> +    unsigned vectors_count;                     /**< number of vectors */
> +};

It would be nice to follow QEMU coding style:
typedef struct IvshmemClientPeer {
    ...
} IvshmemClientPeer;

(Use scripts/checkpatch.pl to check coding style)

> +/* browse the queue, allowing to remove/free the current element */
> +#define    TAILQ_FOREACH_SAFE(var, var2, head, field)            \
> +    for ((var) = TAILQ_FIRST((head)),                            \
> +             (var2) = ((var) ? TAILQ_NEXT((var), field) : NULL); \
> +         (var);                                                  \
> +         (var) = (var2),                                         \
> +             (var2) = ((var2) ? TAILQ_NEXT((var2), field) : NULL))

Please reuse include/qemu/queue.h.  It's a copy of the BSD <sys/queue.h>
and it has QTAILQ_FOREACH_SAFE().

> +    ret = sendmsg(sock_fd, &msg, 0);
> +    if (ret <= 0) {
> +        return -1;
> +    }

This is a blocking sendmsg(2) so it could hang the server if sock_fd's
sndbuf fills up.  This shouldn't happen since the amount of data that
gets sent in the lifetime of a session is relatively small, but there is
a chance.

If hung clients should not be able to block the server then sock_fd
needs to be non-blocking.

> +struct ivshmem_server {
> +    char unix_sock_path[PATH_MAX];  /**< path to unix socket */
> +    int sock_fd;                    /**< unix sock file descriptor */
> +    char shm_path[PATH_MAX];        /**< path to shm */
> +    size_t shm_size;                /**< size of shm */
> +    int shm_fd;                     /**< shm file descriptor */
> +    unsigned n_vectors;             /**< number of vectors */
> +    long cur_id;                    /**< id to be given to next client */
> +    int verbose;                    /**< true in verbose mode */

C99 bool is fine to use in QEMU code.  It makes the code easier to read
because you can be sure something is just true/false and not a bitmap or
integer counter.

> +/* parse the size of shm */
> +static int
> +parse_size(const char *val_str, size_t *val)

Looks similar to QEMU's util/qemu-option.c:parse_option_size().

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]