[PATCH v3 0/3]

[PATCH v3 0/3]

[dje--- via]

Add support for ipv6 host forwarding

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]

Changes from v2:
- split out libslirp commit
- clarify spelling of ipv6 addresses in docs
- tighten parsing of ipv6 addresses

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

Doug Evans (3):
  slirp: Placeholder for libslirp ipv6 hostfwd support
  net/slirp.c: Refactor address parsing
  net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

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

-- 
2.30.0.365.g02bc693789-goog

[PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[dje--- via]

This commit is intended to only contain the slirp submodule change
that adds ipv6 hostfwd support.
---
 slirp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 v3 2/3] net/slirp.c: Refactor address parsing

[dje--- via]

... in preparation for adding ipv6 host forwarding support.

Signed-off-by: Doug Evans <dje@google.com>
---
 net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 129 insertions(+), 71 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
-- 
2.30.0.365.g02bc693789-goog

[PATCH v3 3/3] 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     |  32 +++++++++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 128 +++++++++++++++++++++++++++++++++++++++++++-
 qapi/net.json       |   4 ++
 4 files changed, 164 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..6a9ec0361d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1392,6 +1392,38 @@ 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).
+  Note that IPV6 addresses must be wrapped in square brackets [].
+  E.g., write "[::1]" not "::1".
+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.
+  Note that IPV6 addresses must be wrapped in square brackets [].
+  E.g., write "[::1]" not "::1".
+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..2a59c20286 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,58 @@ 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;
+    }
+
+    if (*p++ != addr_sep) {
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 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)
 {
@@ -743,7 +802,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 +824,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 +876,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 +948,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 +960,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 +1271,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 v3 0/3]

[no-reply]

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



Hi,

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

Type: series
Message-id: 20210203233539.1990032-1-dje@google.com
Subject: [PATCH v3 0/3]

=== 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
 - [tag update]      patchew/20210203213729.1940893-1-dje@google.com -> patchew/20210203213729.1940893-1-dje@google.com
 * [new tag]         patchew/20210203233539.1990032-1-dje@google.com -> patchew/20210203233539.1990032-1-dje@google.com
Switched to a new branch 'test'
998dd12 net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands
e6a45e5 net/slirp.c: Refactor address parsing
6faf9a9 slirp: Placeholder for libslirp ipv6 hostfwd support

=== OUTPUT BEGIN ===
1/3 Checking commit 6faf9a93b1cf (slirp: Placeholder for libslirp ipv6 hostfwd support)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 2 lines checked

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

2/3 Checking commit e6a45e50689f (net/slirp.c: Refactor address parsing)
3/3 Checking commit 998dd12e51aa (net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands)
ERROR: line over 90 characters
#145: FILE: net/slirp.c:748:
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 address", kind);

total: 1 errors, 0 warnings, 250 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210203233539.1990032-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 v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> Add support for ipv6 host forwarding
> 
> 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.

Before I noticed this v3, I send a reply to your v2 sugesting
that we don't need to add any new commands/options. We can
use existing inet_parse() helper function to parse the address
info and transparently support IPv4/6 in the existing commands
and options. This matches normal practice elsewhere in QEMU
for IP dual stack.


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 v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Eric Blake]

On 2/3/21 5:35 PM, dje--- via wrote:
> This commit is intended to only contain the slirp submodule change
> that adds ipv6 hostfwd support.


Missing your signed-off-by, and as written, your authorship would be
based on the From: dje--- via <qemu-devel@nongnu.org> header (that looks
like our mailing list rewrote things due to SPF policies, but that it
botched your name in the process), rather than your Reply-to: Doug Evans
<dje@google.com> header.  To fix the latter, you can convince git to
include a From: line in the first line of the body that will override
whatever is in the headers even after mailing list rewrites.

> ---
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp b/slirp
> index 8f43a99191..358c0827d4 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Doug Evans]

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

On Thu, Feb 4, 2021 at 8:02 AM Eric Blake <eblake@redhat.com> wrote:

> On 2/3/21 5:35 PM, dje--- via wrote:
> > This commit is intended to only contain the slirp submodule change
> > that adds ipv6 hostfwd support.
>
>
> Missing your signed-off-by, and as written, your authorship would be
> based on the From: dje--- via <qemu-devel@nongnu.org> header (that looks
> like our mailing list rewrote things due to SPF policies, but that it
> botched your name in the process), rather than your Reply-to: Doug Evans
> <dje@google.com> header.  To fix the latter, you can convince git to
> include a From: line in the first line of the body that will override
> whatever is in the headers even after mailing list rewrites.
>
> > ---
> >  slirp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/slirp b/slirp
> > index 8f43a99191..358c0827d4 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> > +Subproject commit 358c0827d49778f016312bfb4167fe639900681f
> >


Samuel, how do merges from the libslirp tree into the qemu tree work?
I wasn't expecting there was anything more I would do, but please correct
me if I'm wrong.

Therefore, what this patch (1/3) is is just a placeholder to solve the
problem of removing the "subproject comment" out of the other patches.
The signed-off-by line is intentionally missing.

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

Re: [PATCH v3 1/3] slirp: Placeholder for libslirp ipv6 hostfwd support

[Marc-André Lureau]

Hi,

On Thu, Feb 4, 2021 at 3:38 AM dje--- via <qemu-devel@nongnu.org> wrote:
>
> This commit is intended to only contain the slirp submodule change
> that adds ipv6 hostfwd support.
> ---
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

It's generally recommended to include the shortlog of some sort in the
commit message of what changed between the two commits, ex:
https://patchew.org/QEMU/20210125073427.3970606-1-marcandre.lureau@redhat.com/20210125073427.3970606-2-marcandre.lureau@redhat.com/

thanks

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


-- 
Marc-André Lureau

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > Add support for ipv6 host forwarding
> >
> > 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.
>
> Before I noticed this v3, I send a reply to your v2 sugesting
> that we don't need to add any new commands/options. We can
> use existing inet_parse() helper function to parse the address
> info and transparently support IPv4/6 in the existing commands
> and options. This matches normal practice elsewhere in QEMU
> for IP dual stack.
>

I'm all for this, fwiw.

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

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:

> On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
>> > Add support for ipv6 host forwarding
>> >
>> > 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.
>>
>> Before I noticed this v3, I send a reply to your v2 sugesting
>> that we don't need to add any new commands/options. We can
>> use existing inet_parse() helper function to parse the address
>> info and transparently support IPv4/6 in the existing commands
>> and options. This matches normal practice elsewhere in QEMU
>> for IP dual stack.
>>
>
> I'm all for this, fwiw.
>


I should say I'm all for not adding new commands/options.
Looking at inet_parse() it cannot be used as-is.
The question then becomes: Will refactoring it buy enough?

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

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> 
> > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> >> > Add support for ipv6 host forwarding
> >> >
> >> > 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.
> >>
> >> Before I noticed this v3, I send a reply to your v2 sugesting
> >> that we don't need to add any new commands/options. We can
> >> use existing inet_parse() helper function to parse the address
> >> info and transparently support IPv4/6 in the existing commands
> >> and options. This matches normal practice elsewhere in QEMU
> >> for IP dual stack.
> >>
> >
> > I'm all for this, fwiw.
> >
> 
> 
> I should say I'm all for not adding new commands/options.
> Looking at inet_parse() it cannot be used as-is.
> The question then becomes: Will refactoring it buy enough?

What's the problem your hitting with inet_parse ?

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 v3 0/3]

[Doug Evans]

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

On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> >
> > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > > wrote:
> > >
> > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > >> > Add support for ipv6 host forwarding
> > >> >
> > >> > 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.
> > >>
> > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > >> that we don't need to add any new commands/options. We can
> > >> use existing inet_parse() helper function to parse the address
> > >> info and transparently support IPv4/6 in the existing commands
> > >> and options. This matches normal practice elsewhere in QEMU
> > >> for IP dual stack.
> > >>
> > >
> > > I'm all for this, fwiw.
> > >
> >
> >
> > I should say I'm all for not adding new commands/options.
> > Looking at inet_parse() it cannot be used as-is.
> > The question then becomes: Will refactoring it buy enough?
>
> What's the problem your hitting with inet_parse ?
>


First, this is the inet_parse() function we're talking about, right?

int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)

https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

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

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > >
> > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> > >
> > > > wrote:
> > > >
> > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > >> > Add support for ipv6 host forwarding
> > > >> >
> > > >> > 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.
> > > >>
> > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > >> that we don't need to add any new commands/options. We can
> > > >> use existing inet_parse() helper function to parse the address
> > > >> info and transparently support IPv4/6 in the existing commands
> > > >> and options. This matches normal practice elsewhere in QEMU
> > > >> for IP dual stack.
> > > >>
> > > >
> > > > I'm all for this, fwiw.
> > > >
> > >
> > >
> > > I should say I'm all for not adding new commands/options.
> > > Looking at inet_parse() it cannot be used as-is.
> > > The question then becomes: Will refactoring it buy enough?
> >
> > What's the problem your hitting with inet_parse ?
> >
> 
> 
> First, this is the inet_parse() function we're talking about, right?
> 
> int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> 
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

Yes, that's right.

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 v3 0/3]

[Doug Evans]

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

On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > >
> > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> berrange@redhat.com
> > > >
> > > > > wrote:
> > > > >
> > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > >> > Add support for ipv6 host forwarding
> > > > >> >
> > > > >> > 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.
> > > > >>
> > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > >> that we don't need to add any new commands/options. We can
> > > > >> use existing inet_parse() helper function to parse the address
> > > > >> info and transparently support IPv4/6 in the existing commands
> > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > >> for IP dual stack.
> > > > >>
> > > > >
> > > > > I'm all for this, fwiw.
> > > > >
> > > >
> > > >
> > > > I should say I'm all for not adding new commands/options.
> > > > Looking at inet_parse() it cannot be used as-is.
> > > > The question then becomes: Will refactoring it buy enough?
> > >
> > > What's the problem your hitting with inet_parse ?
> > >
> >
> >
> > First, this is the inet_parse() function we're talking about, right?
> >
> > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >
> >
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
>
> Yes, that's right.
>


Thanks, just wanted to be sure.

The syntax it supports is not the same as what's needed for host forwarding.
inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
hostfwd: address:port-address:port
If we wanted to have a utility that parsed "address:port" for v4+v6 then
we'd have to split the "address:port" part out of inet_parse.

Plus the way inet_parse() parses the address, which is fine for its
purposes, is with sscanf.
Alas the terminating character is not the same (',' vs '-').
IWBN to retain passing sscanf a constant format string so that the compiler
can catch various errors,
and if one keeps that then any kind of refactor loses some appeal.
[Though one could require all callers to accept either ',' or '-' as the
delimiter.]

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

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 02:40:17PM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > > >
> > > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> > berrange@redhat.com
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > > >> > Add support for ipv6 host forwarding
> > > > > >> >
> > > > > >> > 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.
> > > > > >>
> > > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > > >> that we don't need to add any new commands/options. We can
> > > > > >> use existing inet_parse() helper function to parse the address
> > > > > >> info and transparently support IPv4/6 in the existing commands
> > > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > > >> for IP dual stack.
> > > > > >>
> > > > > >
> > > > > > I'm all for this, fwiw.
> > > > > >
> > > > >
> > > > >
> > > > > I should say I'm all for not adding new commands/options.
> > > > > Looking at inet_parse() it cannot be used as-is.
> > > > > The question then becomes: Will refactoring it buy enough?
> > > >
> > > > What's the problem your hitting with inet_parse ?
> > > >
> > >
> > >
> > > First, this is the inet_parse() function we're talking about, right?
> > >
> > > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > >
> > >
> > https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
> >
> > Yes, that's right.
> >
> 
> 
> Thanks, just wanted to be sure.
> 
> The syntax it supports is not the same as what's needed for host forwarding.
> inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
> hostfwd: address:port-address:port
> If we wanted to have a utility that parsed "address:port" for v4+v6 then
> we'd have to split the "address:port" part out of inet_parse.
> 
> Plus the way inet_parse() parses the address, which is fine for its
> purposes, is with sscanf.
> Alas the terminating character is not the same (',' vs '-').
> IWBN to retain passing sscanf a constant format string so that the compiler
> can catch various errors,
> and if one keeps that then any kind of refactor loses some appeal.
> [Though one could require all callers to accept either ',' or '-' as the
> delimiter.]

I think the key useful part to keep common impl for is the handling
of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
"address:port" part out into a new inet_addr_parse() helper that can be
called from inet_pase and from slirp.

inet_parse() can split on the first ",", and then call inet_addr_parse
on the first segment.

slirp can split on "-", and call inet_addr_parse with both segments.


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 v3 0/3]

[Doug Evans]

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

On Thu, Feb 11, 2021 at 1:12 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> [...]
>
> I think the key useful part to keep common impl for is the handling
> of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
> "address:port" part out into a new inet_addr_parse() helper that can be
> called from inet_pase and from slirp.
>
> inet_parse() can split on the first ",", and then call inet_addr_parse
> on the first segment.
>
> slirp can split on "-", and call inet_addr_parse with both segments.
>


v4 here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06011.html

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