From mboxrd@z Thu Jan 1 00:00:00 1970 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 Message-ID: <20140808145132.GC13382@stefanha-thinkpad.redhat.com> References: <1407488118-11245-1-git-send-email-david.marchand@6wind.com> <1407488118-11245-2-git-send-email-david.marchand@6wind.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="hOcCNbCCxyk/YU74" Cc: qemu-devel@nongnu.org, Olivier Matz , kvm@vger.kernel.org, claudio.fontana@huawei.com, armbru@redhat.com, pbonzini@redhat.com, jani.kokkonen@huawei.com, cam@cs.ualberta.ca To: David Marchand Return-path: Received: from mail-wg0-f49.google.com ([74.125.82.49]:58557 "EHLO mail-wg0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752904AbaHHOvg (ORCPT ); Fri, 8 Aug 2014 10:51:36 -0400 Received: by mail-wg0-f49.google.com with SMTP id k14so5625196wgh.8 for ; Fri, 08 Aug 2014 07:51:34 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1407488118-11245-2-git-send-email-david.marchand@6wind.com> Sender: kvm-owner@vger.kernel.org List-ID: --hOcCNbCCxyk/YU74 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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(). --hOcCNbCCxyk/YU74 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT5OP0AAoJEJykq7OBq3PIA10H/j9hIZ4ys8R5KqjdG/4kIf9u Uyyh2ie6JJsFO2bsScheZq+nhE+FCSLZeO6N8pxqApFDIV7u/YsgEvdFTCh6tCPp rRdgo2TZ03mx0o1Adm0StnlfaYGPEHuvhsqpPu9uaZi6RG/KjMoSzsVRAjsqNpz/ wGnj4KHRX6NAApetNw6ACEjaYHlX/8SBkmPsXGjtlfatbHyDqQGOBPLRj74gPg/K ce2YU6HJR6ktgRqipAfV/cQRHYMfNYgKy0pSXoJ6nR/hHk9CF3r/O9YVYCM2dfHl n/ZlDbUwNr5Th77wL9Y5+O8XV29EdNuYpTPxI8wa3yDaQDj+7FdvkQ8oXorz/kI= =pS6a -----END PGP SIGNATURE----- --hOcCNbCCxyk/YU74-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47458) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFlWB-0006h0-IT for qemu-devel@nongnu.org; Fri, 08 Aug 2014 10:51:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XFlW4-0001S3-CO for qemu-devel@nongnu.org; Fri, 08 Aug 2014 10:51:43 -0400 Received: from mail-wi0-x230.google.com ([2a00:1450:400c:c05::230]:63794) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XFlW4-0001Rk-1c for qemu-devel@nongnu.org; Fri, 08 Aug 2014 10:51:36 -0400 Received: by mail-wi0-f176.google.com with SMTP id bs8so1154621wib.15 for ; Fri, 08 Aug 2014 07:51:34 -0700 (PDT) Date: Fri, 8 Aug 2014 15:51:32 +0100 From: Stefan Hajnoczi Message-ID: <20140808145132.GC13382@stefanha-thinkpad.redhat.com> References: <1407488118-11245-1-git-send-email-david.marchand@6wind.com> <1407488118-11245-2-git-send-email-david.marchand@6wind.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="hOcCNbCCxyk/YU74" Content-Disposition: inline In-Reply-To: <1407488118-11245-2-git-send-email-david.marchand@6wind.com> Subject: Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Marchand Cc: Olivier Matz , kvm@vger.kernel.org, claudio.fontana@huawei.com, armbru@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, jani.kokkonen@huawei.com, cam@cs.ualberta.ca --hOcCNbCCxyk/YU74 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 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(). --hOcCNbCCxyk/YU74 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT5OP0AAoJEJykq7OBq3PIA10H/j9hIZ4ys8R5KqjdG/4kIf9u Uyyh2ie6JJsFO2bsScheZq+nhE+FCSLZeO6N8pxqApFDIV7u/YsgEvdFTCh6tCPp rRdgo2TZ03mx0o1Adm0StnlfaYGPEHuvhsqpPu9uaZi6RG/KjMoSzsVRAjsqNpz/ wGnj4KHRX6NAApetNw6ACEjaYHlX/8SBkmPsXGjtlfatbHyDqQGOBPLRj74gPg/K ce2YU6HJR6ktgRqipAfV/cQRHYMfNYgKy0pSXoJ6nR/hHk9CF3r/O9YVYCM2dfHl n/ZlDbUwNr5Th77wL9Y5+O8XV29EdNuYpTPxI8wa3yDaQDj+7FdvkQ8oXorz/kI= =pS6a -----END PGP SIGNATURE----- --hOcCNbCCxyk/YU74--