Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Benjamin]

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(-)

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"
-- 
1.7.3.5

Is this better? Sent the patch by hand since I didn't configure my mail
client yet. Will do for the next patches.

Thanks for your help,

Benjamin

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Stefan Hajnoczi]

On Thu, Nov 24, 2011 at 1:02 AM, Benjamin <mlspirat42@gmail.com> wrote:
>
> Signed-off-by: Benjamin MARSILI <marsil_b@epitech.eu>
> ---

This is the '---' that people typically add comments after.  Git will
not include anything from '---' to the next 'diff --git ...' line.

>  net.c           |    6 ++++-
>  net/socket.c    |   71
> +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-options.hx |    2 +
>  3 files changed, 76 insertions(+), 3 deletions(-)

For example, this automatically inserted diffstat will not be part of
the git commit and it gets applied since it is after the '---'.

> 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"

The GMail web interface always wraps lines.  This breaks the patch and
it cannot be applied without manually fixing it up.  There is no way
to disable this in the GMail web interface but you can use SMTP to
send mails directly and they will not be touched.

Configuring git-send-email for GMail is described here:
http://morefedora.blogspot.com/2009/02/configuring-git-send-email-to-use-gmail.html

Also, please send a top-level mail and not a reply to the existing
thread.  This makes it easier for reviewers and maintainers to spot
your new patch revision.

Stefan

[Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Benjamin]

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(-)

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"
-- 
1.7.3.5

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Benjamin]

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

Re: [Qemu-devel] [PATCH v2] Support for UDP unicast network backend

[Benjamin]

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