diff mbox

[v4] net: add the support for -netdev socket, listen

Message ID 1338989183-28869-1-git-send-email-zwu.kernel@gmail.com
State New
Headers show

Commit Message

Zhiyong Wu June 6, 2012, 1:26 p.m. UTC 
From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

The -net socket,listen option does not work with the newer -netdev
syntax:
 http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01508.html

This patch makes it work now.

For the case where one vlan has multiple listenning sockets,
the patch will also provide the support.

Supported syntax:
 1.) -net socket,listen=127.0.0.1:1234,vlan=0
 2.) -net socket,listen=127.0.0.1:1234,vlan=0 -net socket,listen=127.0.0.1:1235,vlan=0
 3.) -netdev socket,listen=127.0.0.1:1234,id=socket0

Suggested-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 net.c        |   24 +++++++++++
 net.h        |    3 +
 net/socket.c |  123 ++++++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 117 insertions(+), 33 deletions(-)

Comments

Stefan Hajnoczi June 7, 2012, 10:08 a.m. UTC | #1 
On Wed, Jun 06, 2012 at 09:26:23PM +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> The -net socket,listen option does not work with the newer -netdev
> syntax:
>  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01508.html
> 
> This patch makes it work now.
> 
> For the case where one vlan has multiple listenning sockets,
> the patch will also provide the support.
> 
> Supported syntax:
>  1.) -net socket,listen=127.0.0.1:1234,vlan=0
>  2.) -net socket,listen=127.0.0.1:1234,vlan=0 -net socket,listen=127.0.0.1:1235,vlan=0
>  3.) -netdev socket,listen=127.0.0.1:1234,id=socket0
> 
> Suggested-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  net.c        |   24 +++++++++++
>  net.h        |    3 +
>  net/socket.c |  123 ++++++++++++++++++++++++++++++++++++++++++---------------
>  3 files changed, 117 insertions(+), 33 deletions(-)

Please include changelogs in new versions of patches.

It looks like you have implemented "1 connection at a time" semantics.
This is good, I think it maps best to the netdev peer model.  Allowing
multiple clients to connect to a single listen socket at the same time
doesn't fit into the netdev peer model.

I think the patch can be simplified a lot though.  There's no need to
modify net.c or add consumed booleans.

Instead, drop the NetSocketListenState struct and add a listen_fd field
to NetSocketState.  When a -netdev socket,listen= instance is created
there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
net_socket_accept() handler waits for listen_fd to become readable and
then accepts the connection.  When this state transition happens, we no
longer monitor listen_fd for incoming connections...until the client
disconnects again.

This approach doesn't need to change net.c or VLANClientState.  It also
makes memory allocation simpler because we only have 1 struct:
NetSocketState.

Stefan
Zhiyong Wu June 7, 2012, 12:49 p.m. UTC | #2 
On Thu, Jun 7, 2012 at 6:08 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> On Wed, Jun 06, 2012 at 09:26:23PM +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> The -net socket,listen option does not work with the newer -netdev
>> syntax:
>>  http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg01508.html
>>
>> This patch makes it work now.
>>
>> For the case where one vlan has multiple listenning sockets,
>> the patch will also provide the support.
>>
>> Supported syntax:
>>  1.) -net socket,listen=127.0.0.1:1234,vlan=0
>>  2.) -net socket,listen=127.0.0.1:1234,vlan=0 -net socket,listen=127.0.0.1:1235,vlan=0
>>  3.) -netdev socket,listen=127.0.0.1:1234,id=socket0
>>
>> Suggested-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  net.c        |   24 +++++++++++
>>  net.h        |    3 +
>>  net/socket.c |  123 ++++++++++++++++++++++++++++++++++++++++++---------------
>>  3 files changed, 117 insertions(+), 33 deletions(-)
>
> Please include changelogs in new versions of patches.
>
> It looks like you have implemented "1 connection at a time" semantics.
> This is good, I think it maps best to the netdev peer model.  Allowing
> multiple clients to connect to a single listen socket at the same time
> doesn't fit into the netdev peer model.
>
> I think the patch can be simplified a lot though.  There's no need to
> modify net.c or add consumed booleans.
>
> Instead, drop the NetSocketListenState struct and add a listen_fd field
> to NetSocketState.  When a -netdev socket,listen= instance is created
> there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
Have you considered the case where there're mulitple -net
socket,listen= instance in one vlan?
> net_socket_accept() handler waits for listen_fd to become readable and
> then accepts the connection.  When this state transition happens, we no
> longer monitor listen_fd for incoming connections...until the client
> disconnects again.
>
> This approach doesn't need to change net.c or VLANClientState.  It also
> makes memory allocation simpler because we only have 1 struct:
> NetSocketState.
>
> Stefan
>
Paolo Bonzini June 7, 2012, 2:16 p.m. UTC | #3 
Il 07/06/2012 14:49, Zhi Yong Wu ha scritto:
>> > Instead, drop the NetSocketListenState struct and add a listen_fd field
>> > to NetSocketState.  When a -netdev socket,listen= instance is created
>> > there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
> Have you considered the case where there're mulitple -net
> socket,listen= instance in one vlan?

Why should that matter?  They will have different NetSocketState and
different listen_fds.  Each socket will discard incoming packets until
the other side connects.

Paolo
Zhiyong Wu June 7, 2012, 2:50 p.m. UTC | #4 
On Thu, Jun 7, 2012 at 10:16 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/06/2012 14:49, Zhi Yong Wu ha scritto:
>>> > Instead, drop the NetSocketListenState struct and add a listen_fd field
>>> > to NetSocketState.  When a -netdev socket,listen= instance is created
>>> > there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
>> Have you considered the case where there're mulitple -net
>> socket,listen= instance in one vlan?
>
> Why should that matter?  They will have different NetSocketState and
> different listen_fds.  Each socket will discard incoming packets until
> the other side connects.
You are correct, thanks, please see next version.
>
> Paolo
>
Stefan Hajnoczi June 7, 2012, 2:52 p.m. UTC | #5 
On Thu, Jun 7, 2012 at 3:16 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 07/06/2012 14:49, Zhi Yong Wu ha scritto:
>>> > Instead, drop the NetSocketListenState struct and add a listen_fd field
>>> > to NetSocketState.  When a -netdev socket,listen= instance is created
>>> > there will be a NetSocketState with fd=-1 and a valid listen_fd.  The
>> Have you considered the case where there're mulitple -net
>> socket,listen= instance in one vlan?
>
> Why should that matter?  They will have different NetSocketState and
> different listen_fds.  Each socket will discard incoming packets until
> the other side connects.

Exactly, the key is that each -netdev has its own state struct.

Stefan
diff mbox

Patch

diff --git a/net.c b/net.c
index 1922d8a..0114537 100644
--- a/net.c
+++ b/net.c
@@ -190,6 +190,30 @@  static ssize_t qemu_deliver_packet_iov(VLANClientState *sender,
                                        int iovcnt,
                                        void *opaque);
 
+VLANClientState *qemu_lookup_net_client(VLANState *vlan,
+                                        const char *name)
+{
+    VLANClientState *vc = NULL;
+
+    if (vlan) {
+        QTAILQ_FOREACH(vc, &vlan->clients, next) {
+            if ((vc->info->type == NET_CLIENT_TYPE_SOCKET)
+                && (!vc->consumed)) {
+                return vc;
+            }
+        }
+    } else {
+        QTAILQ_FOREACH(vc, &non_vlan_clients, next) {
+            if (!strcmp(vc->name, name)
+                && (!vc->consumed)) {
+                return vc;
+            }
+        }
+    }
+
+    return NULL;
+}
+
 VLANClientState *qemu_new_net_client(NetClientInfo *info,
                                      VLANState *vlan,
                                      VLANClientState *peer,
diff --git a/net.h b/net.h
index 64993b4..6033f43 100644
--- a/net.h
+++ b/net.h
@@ -72,6 +72,7 @@  struct VLANClientState {
     char *name;
     char info_str[256];
     unsigned receive_disabled : 1;
+    bool consumed;
 };
 
 typedef struct NICState {
@@ -90,6 +91,8 @@  struct VLANState {
 
 VLANState *qemu_find_vlan(int id, int allocate);
 VLANClientState *qemu_find_netdev(const char *id);
+VLANClientState *qemu_lookup_net_client(VLANState *vlan,
+                                        const char *name);
 VLANClientState *qemu_new_net_client(NetClientInfo *info,
                                      VLANState *vlan,
                                      VLANClientState *peer,
diff --git a/net/socket.c b/net/socket.c
index 0bcf229..2ddca64 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -32,8 +32,21 @@ 
 #include "qemu-option.h"
 #include "qemu_socket.h"
 
+#define NET_SOCKET_CONNECT        0x0001
+#define NET_SOCKET_LISTEN         0x0002
+#define NET_SOCKET_CREATE         0x0004
+
+typedef struct NetSocketListenState {
+    VLANState *vlan;
+    char *model;
+    char *name;
+    int fd;
+    bool consumed;
+} NetSocketListenState;
+
 typedef struct NetSocketState {
     VLANClientState nc;
+    NetSocketListenState *nls;
     int fd;
     int state; /* 0 = getting length, 1 = getting data */
     unsigned int index;
@@ -42,13 +55,6 @@  typedef struct NetSocketState {
     struct sockaddr_in dgram_dst; /* contains inet host and port destination iff connectionless (SOCK_DGRAM) */
 } NetSocketState;
 
-typedef struct NetSocketListenState {
-    VLANState *vlan;
-    char *model;
-    char *name;
-    int fd;
-} NetSocketListenState;
-
 /* XXX: we consider we can send the whole packet without blocking */
 static ssize_t net_socket_receive(VLANClientState *nc, const uint8_t *buf, size_t size)
 {
@@ -86,6 +92,17 @@  static void net_socket_send(void *opaque)
     eoc:
         qemu_set_fd_handler(s->fd, NULL, NULL, NULL);
         closesocket(s->fd);
+        s->fd = 0;
+        s->state = 0;
+        s->index = 0;
+        s->packet_len = 0;
+        memset(s->buf, 0, sizeof(s->buf));
+        if (s->nls) {
+            s->nls->consumed = false;
+            s->nc.consumed = false;
+            memset(s->nc.info_str, 0, sizeof(s->nc.info_str));
+        }
+
         return;
     }
     buf = buf1;
@@ -247,7 +264,7 @@  static NetClientInfo net_dgram_socket_info = {
 static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
                                                 const char *model,
                                                 const char *name,
-                                                int fd, int is_connected)
+                                                int fd, int flag)
 {
     struct sockaddr_in saddr;
     int newfd;
@@ -260,7 +277,7 @@  static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
      * by ONLY ONE process: we must "clone" this dgram socket --jjo
      */
 
-    if (is_connected) {
+    if (flag & NET_SOCKET_CONNECT) {
         if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
             /* must be bound */
             if (saddr.sin_addr.s_addr == 0) {
@@ -290,7 +307,7 @@  static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
 
     snprintf(nc->info_str, sizeof(nc->info_str),
             "socket: fd=%d (%s mcast=%s:%d)",
-            fd, is_connected ? "cloned" : "",
+            fd, flag & NET_SOCKET_CONNECT ? "cloned" : "",
             inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
 
     s = DO_UPCAST(NetSocketState, nc, nc);
@@ -300,7 +317,9 @@  static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan,
     qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s);
 
     /* mcast: save bound address as dst */
-    if (is_connected) s->dgram_dst=saddr;
+    if (flag & NET_SOCKET_CONNECT) {
+        s->dgram_dst = saddr;
+    }
 
     return s;
 
@@ -325,20 +344,31 @@  static NetClientInfo net_socket_info = {
 static NetSocketState *net_socket_fd_init_stream(VLANState *vlan,
                                                  const char *model,
                                                  const char *name,
-                                                 int fd, int is_connected)
+                                                 int fd, int flag)
 {
     VLANClientState *nc;
     NetSocketState *s;
 
-    nc = qemu_new_net_client(&net_socket_info, vlan, NULL, model, name);
-
-    snprintf(nc->info_str, sizeof(nc->info_str), "socket: fd=%d", fd);
+    if (flag & NET_SOCKET_CREATE) {
+        nc = qemu_new_net_client(&net_socket_info, vlan, NULL, model, name);
+    } else {
+        nc = qemu_lookup_net_client(vlan, name);
+        if (!nc) {
+            return NULL;
+        }
+    }
 
     s = DO_UPCAST(NetSocketState, nc, nc);
 
+    if (flag & NET_SOCKET_LISTEN) {
+        return s;
+    }
+
+    snprintf(nc->info_str, sizeof(nc->info_str), "socket: fd=%d", fd);
+
     s->fd = fd;
 
-    if (is_connected) {
+    if (flag & NET_SOCKET_CONNECT) {
         net_socket_connect(s);
     } else {
         qemu_set_fd_handler(s->fd, NULL, net_socket_connect, s);
@@ -348,7 +378,7 @@  static NetSocketState *net_socket_fd_init_stream(VLANState *vlan,
 
 static NetSocketState *net_socket_fd_init(VLANState *vlan,
                                           const char *model, const char *name,
-                                          int fd, int is_connected)
+                                          int fd, int flag)
 {
     int so_type = -1, optlen=sizeof(so_type);
 
@@ -361,13 +391,13 @@  static NetSocketState *net_socket_fd_init(VLANState *vlan,
     }
     switch(so_type) {
     case SOCK_DGRAM:
-        return net_socket_fd_init_dgram(vlan, model, name, fd, is_connected);
+        return net_socket_fd_init_dgram(vlan, model, name, fd, flag);
     case SOCK_STREAM:
-        return net_socket_fd_init_stream(vlan, model, name, fd, is_connected);
+        return net_socket_fd_init_stream(vlan, model, name, fd, flag);
     default:
         /* who knows ... this could be a eg. a pty, do warn and continue as stream */
         fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
-        return net_socket_fd_init_stream(vlan, model, name, fd, is_connected);
+        return net_socket_fd_init_stream(vlan, model, name, fd, flag);
     }
     return NULL;
 }
@@ -378,9 +408,13 @@  static void net_socket_accept(void *opaque)
     NetSocketState *s1;
     struct sockaddr_in saddr;
     socklen_t len;
-    int fd;
+    int fd, flag = 0;
 
     for(;;) {
+        if (s->consumed) {
+            return;
+        }
+
         len = sizeof(saddr);
         fd = qemu_accept(s->fd, (struct sockaddr *)&saddr, &len);
         if (fd < 0 && errno != EINTR) {
@@ -389,8 +423,14 @@  static void net_socket_accept(void *opaque)
             break;
         }
     }
-    s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1);
+
+    flag |= NET_SOCKET_CONNECT;
+    flag &= ~NET_SOCKET_LISTEN & ~NET_SOCKET_CREATE;
+    s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, flag);
     if (s1) {
+        s->consumed = true;
+        s1->nc.link_down = false;
+        s1->nc.consumed = true;
         snprintf(s1->nc.info_str, sizeof(s1->nc.info_str),
                  "socket: connection from %s:%d",
                  inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
@@ -403,8 +443,9 @@  static int net_socket_listen_init(VLANState *vlan,
                                   const char *host_str)
 {
     NetSocketListenState *s;
-    int fd, val, ret;
+    int fd, val, ret, flag = 0;
     struct sockaddr_in saddr;
+    NetSocketState *ns;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;
@@ -441,6 +482,13 @@  static int net_socket_listen_init(VLANState *vlan,
     s->model = g_strdup(model);
     s->name = name ? g_strdup(name) : NULL;
     s->fd = fd;
+
+    flag &= ~NET_SOCKET_CONNECT;
+    flag |= NET_SOCKET_LISTEN | NET_SOCKET_CREATE;
+    ns = net_socket_fd_init(s->vlan, s->model, s->name, fd, flag);
+    ns->nls = s;
+    ns->nc.link_down = true;
+
     qemu_set_fd_handler(fd, net_socket_accept, NULL, s);
     return 0;
 }
@@ -451,7 +499,7 @@  static int net_socket_connect_init(VLANState *vlan,
                                    const char *host_str)
 {
     NetSocketState *s;
-    int fd, connected, ret, err;
+    int fd, ret, err, flag = 0;
     struct sockaddr_in saddr;
 
     if (parse_host_port(&saddr, host_str) < 0)
@@ -464,7 +512,7 @@  static int net_socket_connect_init(VLANState *vlan,
     }
     socket_set_nonblock(fd);
 
-    connected = 0;
+    flag &= ~NET_SOCKET_CONNECT;
     for(;;) {
         ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr));
         if (ret < 0) {
@@ -482,11 +530,14 @@  static int net_socket_connect_init(VLANState *vlan,
                 return -1;
             }
         } else {
-            connected = 1;
+            flag |= NET_SOCKET_CONNECT;
             break;
         }
     }
-    s = net_socket_fd_init(vlan, model, name, fd, connected);
+
+    flag |= NET_SOCKET_CREATE;
+    flag &= ~NET_SOCKET_LISTEN;
+    s = net_socket_fd_init(vlan, model, name, fd, flag);
     if (!s)
         return -1;
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),
@@ -502,7 +553,7 @@  static int net_socket_mcast_init(VLANState *vlan,
                                  const char *localaddr_str)
 {
     NetSocketState *s;
-    int fd;
+    int fd, flag = 0;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
@@ -521,7 +572,9 @@  static int net_socket_mcast_init(VLANState *vlan,
     if (fd < 0)
         return -1;
 
-    s = net_socket_fd_init(vlan, model, name, fd, 0);
+    flag |= NET_SOCKET_CREATE;
+    flag &= ~NET_SOCKET_CONNECT & ~NET_SOCKET_LISTEN;
+    s = net_socket_fd_init(vlan, model, name, fd, flag);
     if (!s)
         return -1;
 
@@ -541,7 +594,7 @@  static int net_socket_udp_init(VLANState *vlan,
                                  const char *lhost)
 {
     NetSocketState *s;
-    int fd, val, ret;
+    int fd, val, ret, flag = 0;
     struct sockaddr_in laddr, raddr;
 
     if (parse_host_port(&laddr, lhost) < 0) {
@@ -572,7 +625,9 @@  static int net_socket_udp_init(VLANState *vlan,
         return -1;
     }
 
-    s = net_socket_fd_init(vlan, model, name, fd, 0);
+    flag |= NET_SOCKET_CREATE;
+    flag &= ~NET_SOCKET_CONNECT & ~NET_SOCKET_LISTEN;
+    s = net_socket_fd_init(vlan, model, name, fd, flag);
     if (!s) {
         return -1;
     }
@@ -591,7 +646,7 @@  int net_init_socket(QemuOpts *opts,
                     VLANState *vlan)
 {
     if (qemu_opt_get(opts, "fd")) {
-        int fd;
+        int fd, flag = 0;
 
         if (qemu_opt_get(opts, "listen") ||
             qemu_opt_get(opts, "connect") ||
@@ -606,7 +661,9 @@  int net_init_socket(QemuOpts *opts,
             return -1;
         }
 
-        if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) {
+        flag |= NET_SOCKET_CREATE | NET_SOCKET_CONNECT;
+        flag &= ~NET_SOCKET_LISTEN;
+        if (!net_socket_fd_init(vlan, "socket", name, fd, flag)) {
             return -1;
         }
     } else if (qemu_opt_get(opts, "listen")) {