Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
On 08/08/2014 04:51 PM, Stefan Hajnoczi wrote: 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 000..eee97c6 --- /dev/null +++ b/contrib/ivshmem-client/Makefile @@ -0,0 +1,29 @@ 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. Ok, done. But in this case, should we really keep the files in contrib/ ? I used this directory but I am not too sure about this. Maybe hw/misc/ivshmem/ would be better ? The rest of your comments have been integrated in my tree. Is it preferred to send fixes aside the original patches ? or should I send a squashed version ? (I suppose the former is better as it is easier to read). Thanks Stefan. -- David Marchand -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
On 08/10/2014 05:57 AM, Gonglei wrote:
+/* 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.
We always need to know the peer, either for a deletion, creation or update.
+if (peer == NULL || peer == client-local) {
+debug_log(client, receive delete for invalid peer %ld,
Missing '\n' ?
Ok.
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 ?.
Ok, replaced malloc/free with g_malloc/g_free in client and server.
+client-notif_cb = notif_cb;
+client-notif_arg = notif_arg;
+client-verbose = verbose;
Missing client-sock_fd = -1; ?
Ok.
+
+/* 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 ?
Because the server will send us an id and a fd == -1 see
ivshmem-server.c send_initial_info().
+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;
Ok, I also updated the server.
+int fd;
+
+if (vector peer-vectors_count) {
Maybe if (vector = peer-vectors_count) , otherwise the
peer-vectors[] array bounds.
Oh yes, good catch.
It should not happen, at the moment, but it is wrong, indeed.
+/* 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?.
It indicates that we could not notify *all* clients.
Thanks Gonglei.
--
David Marchand
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
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 olivier.m...@6wind.com
Signed-off-by: David Marchand david.march...@6wind.com
---
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 000..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 000..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
[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 olivier.m...@6wind.com
Signed-off-by: David Marchand david.march...@6wind.com
---
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 000..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 000..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
Re: [Qemu-devel] [PATCH v3 1/2] contrib: add ivshmem client and server
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 000..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 */
+#defineTAILQ_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().
pgpwcRy_qOcxl.pgp
Description: PGP signature
