[PATCH v2 0/2] Add support for ipv6 host forwarding

[PATCH v2 0/2] Add support for ipv6 host forwarding

[dje--- via]

This patchset takes the original patch from Maxim,
https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
and updates it.

New option: -ipv6-hostfwd

New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove

These are the ipv6 equivalents of their ipv4 counterparts.

The libslirp part of the patch has been committed upstream,
and will require adding it to qemu.
https://gitlab.freedesktop.org/slirp/libslirp/-/commit/0624fbe7d39b5433d7084a5096d1effc1df74e39
[plus the subsequent merge commit]

Change from v1:
- libslirp part is now upstream
- net/slirp.c changes split into two pieces (refactor, add ipv6)
- added docs

Doug Evans (2):
  net/slirp.c: Refactor address parsing
  net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

 hmp-commands.hx     |  28 ++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 311 +++++++++++++++++++++++++++++++++++---------
 qapi/net.json       |   4 +
 slirp               |   2 +-
 5 files changed, 282 insertions(+), 65 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH v2 1/2] net/slirp.c: Refactor address parsing

[dje--- via]

... in preparation for adding ipv6 host forwarding support.
---
 net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
 slirp       |   2 +-
 2 files changed, 130 insertions(+), 72 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index be914c0be0..a21a313302 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -631,15 +631,83 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *id)
     }
 }
 
-void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
+/*
+ * Parse a protocol name of the form "name<sep>".
+ * Valid protocols are "tcp" and "udp". An empty string means "tcp".
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the result in *is_udp.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_protocol(const char *str, int sep, int *is_udp,
+                                  char **errmsg)
+{
+    char buf[10];
+    const char *p = str;
+
+    if (get_str_sep(buf, sizeof(buf), &p, sep) < 0) {
+        *errmsg = g_strdup("Missing protcol name separator");
+        return NULL;
+    }
+
+    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
+        *is_udp = 0;
+    } else if (!strcmp(buf, "udp")) {
+        *is_udp = 1;
+    } else {
+        *errmsg = g_strdup("Bad protcol name");
+        return NULL;
+    }
+
+    return p;
+}
+
+/*
+ * Parse an ipv4 address/port of the form "addr<addr_sep>port<port_sep>".
+ * "kind" is either "host" or "guest" and is included in error messages.
+ * An empty address means INADDR_ANY.
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the results in *addr, *port.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_in4_addr_port(const char *str, const char *kind,
+                                       int addr_sep, int port_sep,
+                                       struct in_addr *addr, int *port,
+                                       char **errmsg)
 {
-    struct in_addr host_addr = { .s_addr = INADDR_ANY };
-    int host_port;
     char buf[256];
-    const char *src_str, *p;
+    const char *p = str;
+
+    if (get_str_sep(buf, sizeof(buf), &p, addr_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s address separator", kind);
+        return NULL;
+    }
+    if (buf[0] == '\0') {
+        addr->s_addr = INADDR_ANY;
+    } else if (!inet_aton(buf, addr)) {
+        *errmsg = g_strdup_printf("Bad %s address", kind);
+        return NULL;
+    }
+
+    if (get_str_sep(buf, sizeof(buf), &p, port_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s port separator", kind);
+        return NULL;
+    }
+    if (qemu_strtoi(buf, NULL, 10, port) < 0 ||
+        *port < 0 || *port > 65535) {
+        *errmsg = g_strdup_printf("Bad %s port", kind);
+        return NULL;
+    }
+
+    return p;
+}
+
+static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
+                                      int family)
+{
+    const char *src_str;
     SlirpState *s;
-    int is_udp = 0;
-    int err;
     const char *arg1 = qdict_get_str(qdict, "arg1");
     const char *arg2 = qdict_get_try_str(qdict, "arg2");
 
@@ -654,38 +722,42 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
         return;
     }
 
-    p = src_str;
-    if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        goto fail_syntax;
-    }
+    int host_port;
+    int is_udp;
+    char *errmsg = NULL;
+    int err;
 
-    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
-        is_udp = 0;
-    } else if (!strcmp(buf, "udp")) {
-        is_udp = 1;
-    } else {
-        goto fail_syntax;
-    }
+    g_assert(src_str != NULL);
+    const char *p = src_str;
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        goto fail_syntax;
-    }
-    if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (qemu_strtoi(p, NULL, 10, &host_port)) {
-        goto fail_syntax;
+    if (family == AF_INET) {
+        struct in_addr host_addr;
+        if (parse_in4_addr_port(p, "host", ':', '\0', &host_addr, &host_port,
+                                &errmsg) == NULL) {
+            goto fail_syntax;
+        }
+        err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
+    } else {
+        g_assert_not_reached();
     }
 
-    err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
-
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
                    err ? "not found" : "removed");
     return;
 
  fail_syntax:
-    monitor_printf(mon, "invalid format\n");
+    monitor_printf(mon, "Invalid format: %s\n", errmsg);
+    g_free(errmsg);
+}
+
+void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_remove_worker(mon, qdict, AF_INET);
 }
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
@@ -694,56 +766,29 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
     struct in_addr guest_addr = { .s_addr = 0 };
     int host_port, guest_port;
     const char *p;
-    char buf[256];
     int is_udp;
-    char *end;
-    const char *fail_reason = "Unknown reason";
+    char *errmsg = NULL;
 
+    g_assert(redir_str != NULL);
     p = redir_str;
-    if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "No : separators";
-        goto fail_syntax;
-    }
-    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
-        is_udp = 0;
-    } else if (!strcmp(buf, "udp")) {
-        is_udp = 1;
-    } else {
-        fail_reason = "Bad protocol name";
-        goto fail_syntax;
-    }
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "Missing : separator";
-        goto fail_syntax;
-    }
-    if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
-        fail_reason = "Bad host address";
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
-        fail_reason = "Bad host port separator";
-        goto fail_syntax;
-    }
-    host_port = strtol(buf, &end, 0);
-    if (*end != '\0' || host_port < 0 || host_port > 65535) {
-        fail_reason = "Bad host port";
+    p = parse_in4_addr_port(p, "host", ':', '-', &host_addr, &host_port,
+                            &errmsg);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "Missing guest address";
+    if (parse_in4_addr_port(p, "guest", ':', '\0', &guest_addr, &guest_port,
+                            &errmsg) == NULL) {
         goto fail_syntax;
     }
-    if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
-        fail_reason = "Bad guest address";
-        goto fail_syntax;
-    }
-
-    guest_port = strtol(p, &end, 0);
-    if (*end != '\0' || guest_port < 1 || guest_port > 65535) {
-        fail_reason = "Bad guest port";
+    if (guest_port == 0) {
+        errmsg = g_strdup("Bad guest port");
         goto fail_syntax;
     }
 
@@ -757,11 +802,12 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 
  fail_syntax:
     error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
-               fail_reason);
+               errmsg);
+    g_free(errmsg);
     return -1;
 }
 
-void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
+static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
 {
     const char *redir_str;
     SlirpState *s;
@@ -775,13 +821,25 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
         s = slirp_lookup(mon, NULL);
         redir_str = arg1;
     }
-    if (s) {
-        Error *err = NULL;
-        if (slirp_hostfwd(s, redir_str, &err) < 0) {
-            error_report_err(err);
-        }
+    if (!s) {
+        return;
     }
 
+    Error *err = NULL;
+    int rc;
+    if (family == AF_INET) {
+        rc = slirp_hostfwd(s, redir_str, &err);
+    } else {
+        g_assert_not_reached();
+    }
+    if (rc < 0) {
+        error_report_err(err);
+    }
+}
+
+void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_add_worker(mon, qdict, AF_INET);
 }
 
 #ifndef _WIN32
diff --git a/slirp b/slirp
index 8f43a99191..358c0827d4 160000
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
+Subproject commit 358c0827d49778f016312bfb4167fe639900681f
-- 
2.30.0.365.g02bc693789-goog

[PATCH v2 2/2] net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

[dje--- via]

These are identical to their ipv4 counterparts, but for ipv6.

Signed-off-by: Doug Evans <dje@google.com>
---
 hmp-commands.hx     |  28 ++++++++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 129 +++++++++++++++++++++++++++++++++++++++++++-
 qapi/net.json       |   4 ++
 4 files changed, 161 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..bd51173472 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1392,6 +1392,34 @@ SRST
   Remove host-to-guest TCP or UDP redirection.
 ERST
 
+#ifdef CONFIG_SLIRP
+    {
+        .name       = "ipv6_hostfwd_add",
+        .args_type  = "arg1:s,arg2:s?",
+        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport-[guestaddr6]:guestport",
+        .help       = "redirect TCP6 or UDP6 connections from host to guest (requires -net user)",
+        .cmd        = hmp_ipv6_hostfwd_add,
+    },
+#endif
+SRST
+``ipv6_hostfwd_add``
+  Redirect TCP6 or UDP6 connections from host to guest (requires -net user).
+ERST
+
+#ifdef CONFIG_SLIRP
+    {
+        .name       = "ipv6_hostfwd_remove",
+        .args_type  = "arg1:s,arg2:s?",
+        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport",
+        .help       = "remove host-to-guest TCP6 or UDP6 redirection",
+        .cmd        = hmp_ipv6_hostfwd_remove,
+    },
+#endif
+SRST
+``ipv6_hostfwd_remove``
+  Remove host-to-guest TCP6 or UDP6 redirection.
+ERST
+
     {
         .name       = "balloon",
         .args_type  = "value:M",
diff --git a/include/net/slirp.h b/include/net/slirp.h
index bad3e1e241..4796a5cd39 100644
--- a/include/net/slirp.h
+++ b/include/net/slirp.h
@@ -29,6 +29,8 @@
 
 void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
+void hmp_ipv6_hostfwd_add(Monitor *mon, const QDict *qdict);
+void hmp_ipv6_hostfwd_remove(Monitor *mon, const QDict *qdict);
 
 void hmp_info_usernet(Monitor *mon, const QDict *qdict);
 
diff --git a/net/slirp.c b/net/slirp.c
index a21a313302..2482dbd36c 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -70,6 +70,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 /* slirp network adapter */
 
 #define SLIRP_CFG_HOSTFWD 1
+#define SLIRP_CFG_IPV6_HOSTFWD 2
 
 struct slirp_config_str {
     struct slirp_config_str *next;
@@ -101,6 +102,8 @@ static QTAILQ_HEAD(, SlirpState) slirp_stacks =
     QTAILQ_HEAD_INITIALIZER(slirp_stacks);
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
+static int slirp_ipv6_hostfwd(SlirpState *s, const char *redir_str,
+                              Error **errp);
 static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp);
 
 #ifndef _WIN32
@@ -586,6 +589,10 @@ static int net_slirp_init(NetClientState *peer, const char *model,
             if (slirp_hostfwd(s, config->str, errp) < 0) {
                 goto error;
             }
+        } else if (config->flags & SLIRP_CFG_IPV6_HOSTFWD) {
+            if (slirp_ipv6_hostfwd(s, config->str, errp) < 0) {
+                goto error;
+            }
         } else {
             if (slirp_guestfwd(s, config->str, errp) < 0) {
                 goto error;
@@ -703,6 +710,59 @@ static const char *parse_in4_addr_port(const char *str, const char *kind,
     return p;
 }
 
+/*
+ * Parse an ipv6 address/port of the form "addr<addr_sep>port<port_sep>".
+ * "kind" is either "host" or "guest" and is included in error messages.
+ * An empty address means in6addr_any.
+ * Returns a pointer to the end of the parsed string on success, and stores
+ * the results in *addr, *port.
+ * Otherwise returns NULL and stores the error message in *errmsg, which must
+ * be freed by the caller.
+ */
+static const char *parse_in6_addr_port(const char *str, const char *kind,
+                                       int addr_sep, int port_sep,
+                                       struct in6_addr *addr, int *port,
+                                       char **errmsg)
+{
+    char buf[256];
+    const char *p = str;
+
+    if (*(p++) != '[') {
+        *errmsg = g_strdup_printf("IPv6 %s address must be enclosed"
+                                  " in square brackets", kind);
+        return NULL;
+    }
+    if (get_str_sep(buf, sizeof(buf), &p, ']') < 0) {
+        *errmsg = g_strdup_printf("IPv6 %s address must be enclosed"
+                                  " in square brackets", kind);
+        return NULL;
+    }
+    if (buf[0] == '\0') {
+        *addr = in6addr_any;
+    } else if (!inet_pton(AF_INET6, buf, addr)) {
+        *errmsg = g_strdup_printf("Bad %s address", kind);
+        return NULL;
+    }
+
+    /* Ignore the part between the ']' and addr_sep. */
+    if (get_str_sep(buf, sizeof(buf), &p, addr_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s address separator", kind);
+        return NULL;
+    }
+
+    if (get_str_sep(buf, sizeof(buf), &p, port_sep) < 0) {
+        *errmsg = g_strdup_printf("Missing %s port separator", kind);
+        return NULL;
+    }
+    if (qemu_strtoi(buf, NULL, 10, port) < 0 ||
+        *port < 0 || *port > 65535) {
+        *errmsg = g_strdup_printf("Bad %s port", kind);
+        return NULL;
+    }
+
+    return p;
+}
+
 static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
                                       int family)
 {
@@ -743,7 +803,12 @@ static void hmp_hostfwd_remove_worker(Monitor *mon, const QDict *qdict,
         }
         err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
     } else {
-        g_assert_not_reached();
+        struct in6_addr host_addr;
+        if (parse_in6_addr_port(p, "host", ':', '\0', &host_addr, &host_port,
+                                &errmsg) == NULL) {
+            goto fail_syntax;
+        }
+        err = slirp_remove_ipv6_hostfwd(s->slirp, is_udp, host_addr, host_port);
     }
 
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
@@ -760,6 +825,11 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
     hmp_hostfwd_remove_worker(mon, qdict, AF_INET);
 }
 
+void hmp_ipv6_hostfwd_remove(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_remove_worker(mon, qdict, AF_INET6);
+}
+
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
     struct in_addr host_addr = { .s_addr = INADDR_ANY };
@@ -807,6 +877,55 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
     return -1;
 }
 
+static int slirp_ipv6_hostfwd(SlirpState *s, const char *redir_str,
+                              Error **errp)
+{
+    struct in6_addr host_addr = in6addr_any;
+    struct in6_addr guest_addr;
+    int host_port, guest_port;
+    const char *p;
+    int is_udp;
+    char *errmsg = NULL;
+
+    memset(&guest_addr, 0, sizeof(guest_addr));
+    g_assert(redir_str != NULL);
+    p = redir_str;
+
+    p = parse_protocol(p, ':', &is_udp, &errmsg);
+    if (p == NULL) {
+        goto fail_syntax;
+    }
+
+    p = parse_in6_addr_port(p, "host", ':', '-', &host_addr, &host_port,
+                            &errmsg);
+    if (p == NULL) {
+        goto fail_syntax;
+    }
+
+    if (parse_in6_addr_port(p, "guest", ':', '\0', &guest_addr, &guest_port,
+                            &errmsg) == NULL) {
+        goto fail_syntax;
+    }
+    if (guest_port == 0) {
+        errmsg = g_strdup("Bad guest port");
+        goto fail_syntax;
+    }
+
+    if (slirp_add_ipv6_hostfwd(s->slirp, is_udp, host_addr, host_port,
+                               guest_addr, guest_port) < 0) {
+        error_setg(errp, "Could not set up host forwarding rule '%s'",
+                   redir_str);
+        return -1;
+    }
+    return 0;
+
+ fail_syntax:
+    error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
+               errmsg);
+    g_free(errmsg);
+    return -1;
+}
+
 static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
 {
     const char *redir_str;
@@ -830,7 +949,7 @@ static void hmp_hostfwd_add_worker(Monitor *mon, const QDict *qdict, int family)
     if (family == AF_INET) {
         rc = slirp_hostfwd(s, redir_str, &err);
     } else {
-        g_assert_not_reached();
+        rc = slirp_ipv6_hostfwd(s, redir_str, &err);
     }
     if (rc < 0) {
         error_report_err(err);
@@ -842,6 +961,11 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
     hmp_hostfwd_add_worker(mon, qdict, AF_INET);
 }
 
+void hmp_ipv6_hostfwd_add(Monitor *mon, const QDict *qdict)
+{
+    hmp_hostfwd_add_worker(mon, qdict, AF_INET6);
+}
+
 #ifndef _WIN32
 
 /* automatic user mode samba server configuration */
@@ -1148,6 +1272,7 @@ int net_init_slirp(const Netdev *netdev, const char *name,
     /* all optional fields are initialized to "all bits zero" */
 
     net_init_slirp_configs(user->hostfwd, SLIRP_CFG_HOSTFWD);
+    net_init_slirp_configs(user->ipv6_hostfwd, SLIRP_CFG_IPV6_HOSTFWD);
     net_init_slirp_configs(user->guestfwd, 0);
 
     ret = net_slirp_init(peer, "user", name, user->q_restrict,
diff --git a/qapi/net.json b/qapi/net.json
index c31748c87f..443473107a 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -161,6 +161,9 @@
 # @hostfwd: redirect incoming TCP or UDP host connections to guest
 #           endpoints
 #
+# @ipv6-hostfwd: redirect incoming IPV6 TCP or UDP host connections to
+#                guest endpoints (since 6.0)
+#
 # @guestfwd: forward guest TCP connections
 #
 # @tftp-server-name: RFC2132 "TFTP server name" string (Since 3.1)
@@ -189,6 +192,7 @@
     '*smb':       'str',
     '*smbserver': 'str',
     '*hostfwd':   ['String'],
+    '*ipv6-hostfwd': ['String'],
     '*guestfwd':  ['String'],
     '*tftp-server-name': 'str' } }
 
-- 
2.30.0.365.g02bc693789-goog

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Wed, Feb 3, 2021 at 1:37 PM Doug Evans <dje@google.com> wrote:

> ... in preparation for adding ipv6 host forwarding support.
> ---
>  net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
>  slirp       |   2 +-
>  2 files changed, 130 insertions(+), 72 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index be914c0be0..a21a313302 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -631,15 +631,83 @@ static SlirpState *slirp_lookup(Monitor *mon, const
> char *id)



Yeah, the Signed-off-by line is missing here. Will add in next version, but
will wait for further comments.

[-- Attachment #2: Type: text/html, Size: 1051 bytes --]

Re: [PATCH v2 0/2] Add support for ipv6 host forwarding

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210203213729.1940893-1-dje@google.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210203213729.1940893-1-dje@google.com
Subject: [PATCH v2 0/2] Add support for ipv6 host forwarding

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210203213729.1940893-1-dje@google.com -> patchew/20210203213729.1940893-1-dje@google.com
Switched to a new branch 'test'
79cd3b1 net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands
750c0df net/slirp.c: Refactor address parsing

=== OUTPUT BEGIN ===
1/2 Checking commit 750c0dfd5e78 (net/slirp.c: Refactor address parsing)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 266 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 79cd3b109c25 (net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210203213729.1940893-1-dje@google.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Samuel Thibault]

Doug Evans, le mer. 03 févr. 2021 13:37:28 -0800, a ecrit:
> ... in preparation for adding ipv6 host forwarding support.

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

except

> diff --git a/slirp b/slirp
> index 8f43a99191..358c0827d4 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 358c0827d49778f016312bfb4167fe639900681f

Which, I would say, deserves its own commit?

Re: [PATCH v2 2/2] net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

[Samuel Thibault]

Doug Evans, le mer. 03 févr. 2021 13:37:29 -0800, a ecrit:
> @@ -1392,6 +1392,34 @@ SRST
>    Remove host-to-guest TCP or UDP redirection.
>  ERST
>  
> +#ifdef CONFIG_SLIRP
> +    {
> +        .name       = "ipv6_hostfwd_add",
> +        .args_type  = "arg1:s,arg2:s?",
> +        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport-[guestaddr6]:guestport",

Perhaps explicit that the IPv6 address should be enclosed with [] ?

> +    /* Ignore the part between the ']' and addr_sep. */
> +    if (get_str_sep(buf, sizeof(buf), &p, addr_sep) < 0) {

Mmm, I would say that we do not want to just ignore it, and rather make
sure that it is empty, so that we can possibly make extensions later
without breaking existing misuse.

Samuel

Re: [PATCH v2 2/2] net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

On Wed, Feb 3, 2021 at 2:20 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Doug Evans, le mer. 03 févr. 2021 13:37:29 -0800, a ecrit:
> > @@ -1392,6 +1392,34 @@ SRST
> >    Remove host-to-guest TCP or UDP redirection.
> >  ERST
> >
> > +#ifdef CONFIG_SLIRP
> > +    {
> > +        .name       = "ipv6_hostfwd_add",
> > +        .args_type  = "arg1:s,arg2:s?",
> > +        .params     = "[netdev_id]
> [tcp|udp]:[hostaddr6]:hostport-[guestaddr6]:guestport",
>
> Perhaps explicit that the IPv6 address should be enclosed with [] ?
>


Yeah, totally open to suggestions for what to write.
I wasn't sure how to do that without getting klunky,


> > +    /* Ignore the part between the ']' and addr_sep. */
> > +    if (get_str_sep(buf, sizeof(buf), &p, addr_sep) < 0) {
>
> Mmm, I would say that we do not want to just ignore it, and rather make
> sure that it is empty, so that we can possibly make extensions later
> without breaking existing misuse.
>


Completely agree.

[-- Attachment #2: Type: text/html, Size: 1897 bytes --]

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

On Wed, Feb 3, 2021 at 2:15 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Doug Evans, le mer. 03 févr. 2021 13:37:28 -0800, a ecrit:
> > ... in preparation for adding ipv6 host forwarding support.
>
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> except
>
> > diff --git a/slirp b/slirp
> > index 8f43a99191..358c0827d4 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> > +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
>
> Which, I would say, deserves its own commit?
>


Yep. Still getting used to patch submission in the presence of submodules
...

[-- Attachment #2: Type: text/html, Size: 1209 bytes --]

Re: [PATCH v2 2/2] net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

[Daniel P. Berrangé]

On Wed, Feb 03, 2021 at 01:37:29PM -0800, dje--- via wrote:
> These are identical to their ipv4 counterparts, but for ipv6.
> 
> Signed-off-by: Doug Evans <dje@google.com>
> ---
>  hmp-commands.hx     |  28 ++++++++++
>  include/net/slirp.h |   2 +
>  net/slirp.c         | 129 +++++++++++++++++++++++++++++++++++++++++++-
>  qapi/net.json       |   4 ++
>  4 files changed, 161 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d4001f9c5d..bd51173472 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1392,6 +1392,34 @@ SRST
>    Remove host-to-guest TCP or UDP redirection.
>  ERST
>  
> +#ifdef CONFIG_SLIRP
> +    {
> +        .name       = "ipv6_hostfwd_add",
> +        .args_type  = "arg1:s,arg2:s?",
> +        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport-[guestaddr6]:guestport",
> +        .help       = "redirect TCP6 or UDP6 connections from host to guest (requires -net user)",
> +        .cmd        = hmp_ipv6_hostfwd_add,
> +    },
> +#endif
> +SRST
> +``ipv6_hostfwd_add``
> +  Redirect TCP6 or UDP6 connections from host to guest (requires -net user).
> +ERST
> +
> +#ifdef CONFIG_SLIRP
> +    {
> +        .name       = "ipv6_hostfwd_remove",
> +        .args_type  = "arg1:s,arg2:s?",
> +        .params     = "[netdev_id] [tcp|udp]:[hostaddr6]:hostport",
> +        .help       = "remove host-to-guest TCP6 or UDP6 redirection",
> +        .cmd        = hmp_ipv6_hostfwd_remove,
> +    },
> +#endif
> +SRST
> +``ipv6_hostfwd_remove``
> +  Remove host-to-guest TCP6 or UDP6 redirection.
> +ERST

DO we really need new commands for this ? It seems to me that we
can reliably distinction IPv4 vs v6 from the address format, and
thus existing commands can be adapted to support both.

This is the way other command line options and monitor commands
work for IPv4 vs IPv6 elsewhere in QEMU, so I think consistency
is beneficial.  We already have the helper method inet_parse()
that can do this parsing.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Philippe Mathieu-Daudé]

Hi Doug,

On 2/3/21 10:37 PM, dje--- via wrote:
> ... in preparation for adding ipv6 host forwarding support.

Please duplicate subject line, else this commit description as it
doesn't make sense.

> ---
>  net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
>  slirp       |   2 +-
>  2 files changed, 130 insertions(+), 72 deletions(-)
> 
...

> diff --git a/slirp b/slirp
> index 8f43a99191..358c0827d4 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> 

When updating submodules, please describe changes (usually -
when possible - a previous commit updating the submodule is
preferred).

I can not apply your patch using either
https://git.qemu.org/git/libslirp.git or
https://gitlab.freedesktop.org/slirp/libslirp.git:

fatal: bad object 358c0827d49778f016312bfb4167fe639900681f

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Doug Evans]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On Mon, Feb 8, 2021 at 3:09 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Hi Doug,
>
> On 2/3/21 10:37 PM, dje--- via wrote:
> > ... in preparation for adding ipv6 host forwarding support.
>
> Please duplicate subject line, else this commit description as it
> doesn't make sense.
>


Hmmm. Is this a bug in git format-patch/send-email?

I agree the current behaviour is suboptimal ... Perhaps there's an option
I'm not adding?
Or does one manually work around this?


> ---
> >  net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
> >  slirp       |   2 +-
> >  2 files changed, 130 insertions(+), 72 deletions(-)
> >
> ...
>
> > diff --git a/slirp b/slirp
> > index 8f43a99191..358c0827d4 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> > +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> >
>
> When updating submodules, please describe changes (usually -
> when possible - a previous commit updating the submodule is
> preferred).
>
> I can not apply your patch using either
> https://git.qemu.org/git/libslirp.git or
> https://gitlab.freedesktop.org/slirp/libslirp.git:
>
> fatal: bad object 358c0827d49778f016312bfb4167fe639900681f
>


I think that's expected until the patch has been merged from libslirp into
qemu 's tree.
Samuel, how do qemu patches involving libslirp changes usually work?
Should I have held off submitting the qemu patch until the libslirp
prerequisite has been added to qemu's tree,
or maybe I should include the libslirp patch so that people can at least
apply it (with a caveat saying the patch is already in libslirp.git) until
it's added to the qemu tree?

[-- Attachment #2: Type: text/html, Size: 3199 bytes --]

Re: [PATCH v2 1/2] net/slirp.c: Refactor address parsing

[Samuel Thibault]

Hello,

Doug Evans, le lun. 08 févr. 2021 10:59:01 -0800, a ecrit:
> Samuel, how do qemu patches involving libslirp changes usually work?

Well, we haven't had many yet actually :)

> Should I have held off submitting the qemu patch until the libslirp
> prerequisite has been added to qemu's tree,

No, as this example shows, there are iterations that can happen on the
qemu side before we have something we can commit, so better do both in
parallel.

I don't know what qemu people think about the slirp submodule: do qemu
prefers to only track stable branchs, or would it be fine to track the
master branch? I guess you'd prefer to have a slirp stable release you
can depend on when releasing qemu, so perhaps better wait for a slirp
release before bumping in qemu, just to be safe? Which doesn't mean
avoiding submitting patchqueues that depend on it before that, better go
in parallel.

> or maybe I should include the libslirp patch so that people can at least apply
> it (with a caveat saying the patch is already in libslirp.git) until it's added
> to the qemu tree?

Not sure what is best here. We at least need something so that people
are not confused by the patchqueue calling some function that doesn't
exist in the submodule yet.

Samuel