diff mbox

[v2] Support for UDP unicast network backend

Message ID 1322225387-15073-1-git-send-email-mlspirat42@gmail.com
State New
Headers show

Commit Message

Benjamin Nov. 25, 2011, 12:49 p.m. UTC 
From: Benjamin MARSILI <marsil_b@epitech.eu>

Signed-off-by: Benjamin MARSILI <marsil_b@epitech.eu>
---
 net.c           |    6 ++++-
 net/socket.c    |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 qemu-options.hx |    2 +
 3 files changed, 76 insertions(+), 3 deletions(-)

Comments

Stefan Hajnoczi Nov. 28, 2011, 11:39 a.m. UTC | #1 
On Fri, Nov 25, 2011 at 12:49 PM, Benjamin <mlspirat42@gmail.com> wrote:
> +    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
> +    if (fd < 0) {
> +        perror("socket(PF_INET, SOCK_DGRAM)");
> +        return -1;
> +    }
> +    val = 1;
> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
> +                   (const char *)&val, sizeof(val));
> +    if (ret < 0) {
> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");

Please avoid leaking the file descriptor on error:
closesocket(fd);

Since existing code also does this it may be more appropriate to send
a follow-up patch that cleans up all of net/socket.c.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Stefan
Stefan Hajnoczi Nov. 29, 2011, 9:47 a.m. UTC | #2 
On Tue, Nov 29, 2011 at 5:29 PM, Benjamin <mlspirat42@gmail.com> wrote:
> On 11/28/11 20:39, Stefan Hajnoczi wrote:
>>
>> On Fri, Nov 25, 2011 at 12:49 PM, Benjamin<mlspirat42@gmail.com>  wrote:
>>>
>>> +    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>>> +    if (fd<  0) {
>>> +        perror("socket(PF_INET, SOCK_DGRAM)");
>>> +        return -1;
>>> +    }
>>> +    val = 1;
>>> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>>> +                   (const char *)&val, sizeof(val));
>>> +    if (ret<  0) {
>>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>>
>>
>> Please avoid leaking the file descriptor on error:
>> closesocket(fd);
>>
>> Since existing code also does this it may be more appropriate to send
>> a follow-up patch that cleans up all of net/socket.c.
>>
>> Reviewed-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>>
>> Stefan
>
>
> I can do that. However is it really a leak considering the fact that
> the program will call exit just after?
> If it's a matter of consistency and coding style I would understand
> though.

net/socket.c should not make assumptions about the main program
exiting after an error.  NICs can be added at runtime using netdev_add
and that should not leak file descriptors.

> One more thing, git-format-patch added a "From" field to the header and
> caused this glitch in the mail. I thought git-send-mail or the mail
> server would handle it well but apparently not:
>
> From 2f5b85fcadcfee3b75a6a21dc86d10b945c99f0f Mon Sep 17 00:00:00 2001
> From: Benjamin MARSILI <marsil_b@epitech.eu>
>
> git-am didn't complain with the patch that I sent but it may break after
> gmail relayed it
> (http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg03152.html).
> The second from header is interpreted as text... Should I remove the
> first "From" field before sending the patch?

This is normal and is not a problem.  Your git commit is authored by
Benjamin MARSILI <marsil_b@epitech.eu> but you sent the mail from
Benjamin <mlspirat42@gmail.com>.

git-am will apply the patch with Benjamin MARSILI
<marsil_b@epitech.eu> as the author and it will forget about Benjamin
<mlspirat42@gmail.com>.  This is usually what you want - it let's you
credit commits to other people but send the patch emails on their
behalf.

Stefan
Benjamin Nov. 29, 2011, 5:29 p.m. UTC | #3 
On 11/28/11 20:39, Stefan Hajnoczi wrote:
> On Fri, Nov 25, 2011 at 12:49 PM, Benjamin<mlspirat42@gmail.com>  wrote:
>> +    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>> +    if (fd<  0) {
>> +        perror("socket(PF_INET, SOCK_DGRAM)");
>> +        return -1;
>> +    }
>> +    val = 1;
>> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>> +                   (const char *)&val, sizeof(val));
>> +    if (ret<  0) {
>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>
> Please avoid leaking the file descriptor on error:
> closesocket(fd);
>
> Since existing code also does this it may be more appropriate to send
> a follow-up patch that cleans up all of net/socket.c.
>
> Reviewed-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>
> Stefan

I can do that. However is it really a leak considering the fact that
the program will call exit just after?
If it's a matter of consistency and coding style I would understand
though.

One more thing, git-format-patch added a "From" field to the header and
caused this glitch in the mail. I thought git-send-mail or the mail
server would handle it well but apparently not:

 From 2f5b85fcadcfee3b75a6a21dc86d10b945c99f0f Mon Sep 17 00:00:00 2001
From: Benjamin MARSILI <marsil_b@epitech.eu>

git-am didn't complain with the patch that I sent but it may break after
gmail relayed it 
(http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg03152.html).
The second from header is interpreted as text... Should I remove the
first "From" field before sending the patch?

Sorry for the noise on the ML and thanks to all those who helped me get
involved.

Benjamin
Benjamin Nov. 29, 2011, 7:45 p.m. UTC | #4 
On 11/29/11 18:47, Stefan Hajnoczi wrote:
> On Tue, Nov 29, 2011 at 5:29 PM, Benjamin<mlspirat42@gmail.com>  wrote:
>> On 11/28/11 20:39, Stefan Hajnoczi wrote:
>>>
>>> On Fri, Nov 25, 2011 at 12:49 PM, Benjamin<mlspirat42@gmail.com>    wrote:
>>>>
>>>> +    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>>>> +    if (fd<    0) {
>>>> +        perror("socket(PF_INET, SOCK_DGRAM)");
>>>> +        return -1;
>>>> +    }
>>>> +    val = 1;
>>>> +    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>>>> +                   (const char *)&val, sizeof(val));
>>>> +    if (ret<    0) {
>>>> +        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
>>>
>>>
>>> Please avoid leaking the file descriptor on error:
>>> closesocket(fd);
>>>
>>> Since existing code also does this it may be more appropriate to send
>>> a follow-up patch that cleans up all of net/socket.c.
>>>
>>> Reviewed-by: Stefan Hajnoczi<stefanha@linux.vnet.ibm.com>
>>>
>>> Stefan
>>
>>
>> I can do that. However is it really a leak considering the fact that
>> the program will call exit just after?
>> If it's a matter of consistency and coding style I would understand
>> though.
>
> net/socket.c should not make assumptions about the main program
> exiting after an error.  NICs can be added at runtime using netdev_add
> and that should not leak file descriptors.
>

Ah, indeed, I mixed up "err" and "perror" since I'm used to calling
"warn" instead of perror.

>> One more thing, git-format-patch added a "From" field to the header and
>> caused this glitch in the mail. I thought git-send-mail or the mail
>> server would handle it well but apparently not:
>>
>>  From 2f5b85fcadcfee3b75a6a21dc86d10b945c99f0f Mon Sep 17 00:00:00 2001
>> From: Benjamin MARSILI<marsil_b@epitech.eu>
>>
>> git-am didn't complain with the patch that I sent but it may break after
>> gmail relayed it
>> (http://lists.gnu.org/archive/html/qemu-devel/2011-11/msg03152.html).
>> The second from header is interpreted as text... Should I remove the
>> first "From" field before sending the patch?
>
> This is normal and is not a problem.  Your git commit is authored by
> Benjamin MARSILI<marsil_b@epitech.eu>  but you sent the mail from
> Benjamin<mlspirat42@gmail.com>.
>
> git-am will apply the patch with Benjamin MARSILI
> <marsil_b@epitech.eu>  as the author and it will forget about Benjamin
> <mlspirat42@gmail.com>.  This is usually what you want - it let's you
> credit commits to other people but send the patch emails on their
> behalf.
>
> Stefan

Great, thank you, sending v3 soon.

Benjamin
diff mbox

Patch

diff --git a/net.c b/net.c
index cb52050..8e957b2 100644
--- a/net.c
+++ b/net.c
@@ -999,7 +999,11 @@  static const struct {
             }, {
                 .name = "localaddr",
                 .type = QEMU_OPT_STRING,
-                .help = "source address for multicast packets",
+                .help = "source address and port for multicast and udp packets",
+            }, {
+                .name = "udp",
+                .type = QEMU_OPT_STRING,
+                .help = "UDP unicast address and port number",
             },
             { /* end of list */ }
         },
diff --git a/net/socket.c b/net/socket.c
index e9ef128..ca183f2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -524,6 +524,55 @@  static int net_socket_mcast_init(VLANState *vlan,
 
 }
 
+static int net_socket_udp_init(VLANState *vlan,
+                                 const char *model,
+                                 const char *name,
+                                 const char *rhost,
+                                 const char *lhost)
+{
+    NetSocketState *s;
+    int fd, val, ret;
+    struct sockaddr_in laddr, raddr;
+
+    if (parse_host_port(&laddr, lhost) < 0) {
+        return -1;
+    }
+
+    if (parse_host_port(&raddr, rhost) < 0) {
+        return -1;
+    }
+
+    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
+    if (fd < 0) {
+        perror("socket(PF_INET, SOCK_DGRAM)");
+        return -1;
+    }
+    val = 1;
+    ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
+                   (const char *)&val, sizeof(val));
+    if (ret < 0) {
+        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
+        return -1;
+    }
+    ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
+    if (ret < 0) {
+        perror("bind");
+        return -1;
+    }
+
+    s = net_socket_fd_init(vlan, model, name, fd, 0);
+    if (!s) {
+        return -1;
+    }
+
+    s->dgram_dst = raddr;
+
+    snprintf(s->nc.info_str, sizeof(s->nc.info_str),
+             "socket: udp=%s:%d",
+             inet_ntoa(raddr.sin_addr), ntohs(raddr.sin_port));
+    return 0;
+}
+
 int net_init_socket(QemuOpts *opts,
                     Monitor *mon,
                     const char *name,
@@ -597,10 +646,28 @@  int net_init_socket(QemuOpts *opts,
         if (net_socket_mcast_init(vlan, "socket", name, mcast, localaddr) == -1) {
             return -1;
         }
+    } else if (qemu_opt_get(opts, "udp")) {
+        const char *udp, *localaddr;
+
+        if (qemu_opt_get(opts, "fd") ||
+            qemu_opt_get(opts, "connect") ||
+            qemu_opt_get(opts, "listen") ||
+            qemu_opt_get(opts, "mcast")) {
+            error_report("fd=, connect=, listen=\
+                         and mcast= is invalid with udp=");
+            return -1;
+        }
+
+        udp = qemu_opt_get(opts, "udp");
+        localaddr = qemu_opt_get(opts, "localaddr");
+
+        if (net_socket_udp_init(vlan, "udp", name, udp, localaddr) == -1) {
+            return -1;
+        }
     } else {
-        error_report("-socket requires fd=, listen=, connect= or mcast=");
+        error_report("-socket requires fd=, listen=, \
+                     connect=, mcast= or udp=");
         return -1;
     }
-
     return 0;
 }
diff --git a/qemu-options.hx b/qemu-options.hx
index 681eaf1..5495368 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1217,6 +1217,8 @@  DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net socket[,vlan=n][,name=str][,fd=h][,mcast=maddr:port[,localaddr=addr]]\n"
     "                connect the vlan 'n' to multicast maddr and port\n"
     "                use 'localaddr=addr' to specify the host address to send packets from\n"
+    "-net socket[,vlan=n][,name=str][,fd=h][,udp=host:port][,localaddr=host:port]\n"
+    "                connect the vlan 'n' to another VLAN using an UDP tunnel\n"
 #ifdef CONFIG_VDE
     "-net vde[,vlan=n][,name=str][,sock=socketpath][,port=n][,group=groupname][,mode=octalmode]\n"
     "                connect the vlan 'n' to port 'n' of a vde switch running\n"