[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server |
Date: |
Fri, 8 Aug 2014 15:51:32 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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().
pgpWc54TEp6po.pgp
Description: PGP signature