[PATCH v4 0/4] Add support for ipv6 host forwarding

[PATCH v4 0/4] Add support for ipv6 host forwarding

[Doug Evans via]

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

Option hostfwd is extended to support ipv6 addresses.
Commands hostfwd_add, hostfwd_remove are extended as well.

The libslirp part of the patch has been committed upstream,
and is now in qemu. See patch 1/4.

Changes from v3:

1/4 slirp: Advance libslirp submodule to add ipv6 host-forward support

- pick up latest libslirp patch to reject ipv6 addr-any for guest address
  - libslirp currently only provides a stateless DHCPv6 server, which means
    it can't know in advance what the guest's IP address is, and thus
    cannot do the "addr-any -> guest ip address" translation that is done
    for ipv4

2/4 util/qemu-sockets.c: Split host:port parsing out of inet_parse

- this patch is new in v4
  - provides new utility: inet_parse_host_and_port, updates inet_parse
    to use it

3/4 net/slirp.c: Refactor address parsing

- this patch renamed from 2/3 to 3/4
- call inet_parse_host_and_port from util/qemu-sockets.c
- added tests/acceptance/hostfwd.py

4/4 net: Extend host forwarding to support IPv6

- this patch renamed from 3/3 to 4/4
- ipv6 support added to existing hostfwd option, commands
  - instead of creating new ipv6 option, commands
- added tests to tests/acceptance/hostfwd.py

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 (4):
  slirp: Advance libslirp submodule to add ipv6 host-forward support
  util/qemu-sockets.c: Split host:port parsing out of inet_parse
  net/slirp.c: Refactor address parsing
  net: Extend host forwarding to support IPv6

 hmp-commands.hx             |  15 +++
 include/qemu/sockets.h      |   3 +
 net/slirp.c                 | 194 ++++++++++++++++++++++++------------
 slirp                       |   2 +-
 tests/acceptance/hostfwd.py | 150 ++++++++++++++++++++++++++++
 util/qemu-sockets.c         |  94 ++++++++++++-----
 6 files changed, 370 insertions(+), 88 deletions(-)
 create mode 100644 tests/acceptance/hostfwd.py

-- 
2.30.0.617.g56c4b15f3c-goog

[PATCH v4 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support

[Doug Evans via]

Signed-off-by: Doug Evans <dje@google.com>
---

Changes from v3:
- pick up latest libslirp patch to reject ipv6 addr-any for guest address
  - libslirp currently only provides a stateless DHCPv6 server, which means
    it can't know in advance what the guest's IP address is, and thus
    cannot do the "addr-any -> guest ip address" translation that is done
    for ipv4

Changes from v2:
- this patch is new in v3, split out from v2

 slirp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/slirp b/slirp
index 8f43a99191..26ae658a83 160000
--- a/slirp
+++ b/slirp
@@ -1 +1 @@
-Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
+Subproject commit 26ae658a83eeca16780cf5615c8247cbb151c3fa
-- 
2.30.0.617.g56c4b15f3c-goog

Re: [PATCH v4 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support

[Daniel P. Berrangé]

On Thu, Feb 18, 2021 at 12:15:35PM -0800, Doug Evans wrote:

FWIW, normally when QEMU updates libslirp, the commit message is
set to contain the "git shortlog old..new" output

> Signed-off-by: Doug Evans <dje@google.com>
> ---
> 
> Changes from v3:
> - pick up latest libslirp patch to reject ipv6 addr-any for guest address
>   - libslirp currently only provides a stateless DHCPv6 server, which means
>     it can't know in advance what the guest's IP address is, and thus
>     cannot do the "addr-any -> guest ip address" translation that is done
>     for ipv4
> 
> Changes from v2:
> - this patch is new in v3, split out from v2
> 
>  slirp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/slirp b/slirp
> index 8f43a99191..26ae658a83 160000
> --- a/slirp
> +++ b/slirp
> @@ -1 +1 @@
> -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> +Subproject commit 26ae658a83eeca16780cf5615c8247cbb151c3fa
> -- 
> 2.30.0.617.g56c4b15f3c-goog
> 

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 v4 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support

[Doug Evans]

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

On Fri, Feb 19, 2021 at 1:38 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Feb 18, 2021 at 12:15:35PM -0800, Doug Evans wrote:
>
> FWIW, normally when QEMU updates libslirp, the commit message is
> set to contain the "git shortlog old..new" output
>


Ah. In this case I'm not sure what to do as QEMU master is using Libslirp
stable-4.2 branch (at least in QEMU's libslirp.git).

Samuel, please let me know what should happen here.
I may need some hand holding to come up with The Right patch to submit.
I think you know what patches are needed here, but I don't know what I
should be submitting in this 1/4 patch of the series.



>
> > Signed-off-by: Doug Evans <dje@google.com>
> > ---
> >
> > Changes from v3:
> > - pick up latest libslirp patch to reject ipv6 addr-any for guest address
> >   - libslirp currently only provides a stateless DHCPv6 server, which
> means
> >     it can't know in advance what the guest's IP address is, and thus
> >     cannot do the "addr-any -> guest ip address" translation that is done
> >     for ipv4
> >
> > Changes from v2:
> > - this patch is new in v3, split out from v2
> >
> >  slirp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/slirp b/slirp
> > index 8f43a99191..26ae658a83 160000
> > --- a/slirp
> > +++ b/slirp
> > @@ -1 +1 @@
> > -Subproject commit 8f43a99191afb47ca3f3c6972f6306209f367ece
> > +Subproject commit 26ae658a83eeca16780cf5615c8247cbb151c3fa
> > --
> > 2.30.0.617.g56c4b15f3c-goog
> >
>
> 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 :|
>
>

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

[PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans via]

The parsing is moved into new function inet_parse_host_and_port.
This is done in preparation for using the function in net/slirp.c.

Signed-off-by: Doug Evans <dje@google.com>
---

Changes from v3:
- this patch is new in v4
  - provides new utility: inet_parse_host_and_port, updates inet_parse
    to use it

 include/qemu/sockets.h |  3 ++
 util/qemu-sockets.c    | 94 +++++++++++++++++++++++++++++++-----------
 2 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7d1f813576..f720378a6b 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -31,6 +31,9 @@ int socket_set_fast_reuse(int fd);
 
 int inet_ai_family_from_address(InetSocketAddress *addr,
                                 Error **errp);
+const char* inet_parse_host_and_port(const char* str, int terminator,
+                                     char **addr, char **port, bool *is_v6,
+                                     Error **errp);
 int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
 int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8af0278f15..9fca7d9212 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -615,44 +615,88 @@ static int inet_parse_flag(const char *flagname, const char *optstr, bool *val,
     return 0;
 }
 
-int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
+/*
+ * Parse an inet host and port as "host:port<terminator>".
+ * Terminator may be '\0'.
+ * The syntax for ipv4 addresses is: address:port.
+ * The syntax for ipv6 addresses is: [address]:port.
+ * On success, returns a pointer to the terminator. Space for the address and
+ * port is malloced and stored in *host, *port, the caller must free.
+ * *is_v6 indicates whether the address is ipv4 or ipv6. If ipv6 then the
+ * surrounding [] brackets are removed.
+ * On failure NULL is returned with the error stored in *errp.
+ */
+const char* inet_parse_host_and_port(const char* str, int terminator,
+                                     char **hostp, char **portp, bool *is_v6,
+                                     Error **errp)
 {
-    const char *optstr, *h;
+    const char *terminator_ptr = strchr(str, terminator);
+    g_autofree char *buf = NULL;
     char host[65];
     char port[33];
-    int to;
-    int pos;
-    char *begin;
 
-    memset(addr, 0, sizeof(*addr));
+    if (terminator_ptr == NULL) {
+        /* If the terminator isn't found then use the entire string. */
+        terminator_ptr = str + strlen(str);
+    }
+    buf = g_strndup(str, terminator_ptr - str);
 
-    /* parse address */
-    if (str[0] == ':') {
-        /* no host given */
-        host[0] = '\0';
-        if (sscanf(str, ":%32[^,]%n", port, &pos) != 1) {
-            error_setg(errp, "error parsing port in address '%s'", str);
-            return -1;
-        }
-    } else if (str[0] == '[') {
+    if (buf[0] == '[') {
         /* IPv6 addr */
-        if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) != 2) {
-            error_setg(errp, "error parsing IPv6 address '%s'", str);
-            return -1;
+        if (buf[1] == ']') {
+            /* sscanf %[ doesn't recognize empty contents. */
+            host[0] = '\0';
+            if (sscanf(buf, "[]:%32s", port) != 1) {
+                error_setg(errp, "error parsing IPv6 host:port '%s'", buf);
+                return NULL;
+            }
+        } else {
+            if (sscanf(buf, "[%64[^]]]:%32s", host, port) != 2) {
+                error_setg(errp, "error parsing IPv6 host:port '%s'", buf);
+                return NULL;
+            }
         }
     } else {
-        /* hostname or IPv4 addr */
-        if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, &pos) != 2) {
-            error_setg(errp, "error parsing address '%s'", str);
-            return -1;
+        if (buf[0] == ':') {
+            /* no host given */
+            host[0] = '\0';
+            if (sscanf(buf, ":%32s", port) != 1) {
+                error_setg(errp, "error parsing host:port '%s'", buf);
+                return NULL;
+            }
+        } else {
+            /* hostname or IPv4 addr */
+            if (sscanf(buf, "%64[^:]:%32s", host, port) != 2) {
+                error_setg(errp, "error parsing host:port '%s'", buf);
+                return NULL;
+            }
         }
     }
 
-    addr->host = g_strdup(host);
-    addr->port = g_strdup(port);
+    *hostp = g_strdup(host);
+    *portp = g_strdup(port);
+    *is_v6 = buf[0] == '[';
+
+    return terminator_ptr;
+}
+
+int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
+{
+    const char *optstr, *h;
+    bool is_v6;
+    int to;
+    int pos;
+    char *begin;
+
+    memset(addr, 0, sizeof(*addr));
+
+    optstr = inet_parse_host_and_port(str, ',', &addr->host, &addr->port,
+                                      &is_v6, errp);
+    if (optstr == NULL) {
+        return -1;
+    }
 
     /* parse options */
-    optstr = str + pos;
     h = strstr(optstr, ",to=");
     if (h) {
         h += 4;
-- 
2.30.0.617.g56c4b15f3c-goog

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Daniel P. Berrangé]

On Thu, Feb 18, 2021 at 12:15:36PM -0800, Doug Evans wrote:
> The parsing is moved into new function inet_parse_host_and_port.
> This is done in preparation for using the function in net/slirp.c.
> 
> Signed-off-by: Doug Evans <dje@google.com>
> ---
> 
> Changes from v3:
> - this patch is new in v4
>   - provides new utility: inet_parse_host_and_port, updates inet_parse
>     to use it
> 
>  include/qemu/sockets.h |  3 ++
>  util/qemu-sockets.c    | 94 +++++++++++++++++++++++++++++++-----------
>  2 files changed, 72 insertions(+), 25 deletions(-)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 7d1f813576..f720378a6b 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -31,6 +31,9 @@ int socket_set_fast_reuse(int fd);
>  
>  int inet_ai_family_from_address(InetSocketAddress *addr,
>                                  Error **errp);
> +const char* inet_parse_host_and_port(const char* str, int terminator,
> +                                     char **addr, char **port, bool *is_v6,
> +                                     Error **errp);
>  int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
>  int inet_connect(const char *str, Error **errp);
>  int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8af0278f15..9fca7d9212 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -615,44 +615,88 @@ static int inet_parse_flag(const char *flagname, const char *optstr, bool *val,
>      return 0;
>  }
>  
> -int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> +/*
> + * Parse an inet host and port as "host:port<terminator>".
> + * Terminator may be '\0'.
> + * The syntax for ipv4 addresses is: address:port.
> + * The syntax for ipv6 addresses is: [address]:port.

It also supports

   "The syntax for hostnames is hostname:port

> + * On success, returns a pointer to the terminator. Space for the address and
> + * port is malloced and stored in *host, *port, the caller must free.
> + * *is_v6 indicates whether the address is ipv4 or ipv6. If ipv6 then the
> + * surrounding [] brackets are removed.

When is_v6 is true, it indicates that a numeric ipv6 address was given.
When false either a numberic ipv4 address or hostname was given.

> + * On failure NULL is returned with the error stored in *errp.
> + */
> +const char* inet_parse_host_and_port(const char* str, int terminator,
> +                                     char **hostp, char **portp, bool *is_v6,
> +                                     Error **errp)
>  {
> -    const char *optstr, *h;
> +    const char *terminator_ptr = strchr(str, terminator);
> +    g_autofree char *buf = NULL;
>      char host[65];
>      char port[33];
> -    int to;
> -    int pos;
> -    char *begin;
>  
> -    memset(addr, 0, sizeof(*addr));
> +    if (terminator_ptr == NULL) {
> +        /* If the terminator isn't found then use the entire string. */
> +        terminator_ptr = str + strlen(str);
> +    }
> +    buf = g_strndup(str, terminator_ptr - str);
>  
> -    /* parse address */
> -    if (str[0] == ':') {
> -        /* no host given */
> -        host[0] = '\0';
> -        if (sscanf(str, ":%32[^,]%n", port, &pos) != 1) {
> -            error_setg(errp, "error parsing port in address '%s'", str);
> -            return -1;
> -        }


> -    } else if (str[0] == '[') {
> +    if (buf[0] == '[') {
>          /* IPv6 addr */
> -        if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) != 2) {
> -            error_setg(errp, "error parsing IPv6 address '%s'", str);
> -            return -1;
> +        if (buf[1] == ']') {
> +            /* sscanf %[ doesn't recognize empty contents. */
> +            host[0] = '\0';
> +            if (sscanf(buf, "[]:%32s", port) != 1) {
> +                error_setg(errp, "error parsing IPv6 host:port '%s'", buf);
> +                return NULL;
> +            }

This is introducing new functionality to the parser. Current callers
let empty string ":port" be used for both ipv4 and ipv6, based
on whether the flags ",ipv4[=on|off],ipv6[=on|off]" later follow.

I presume you want an explicit way to represent an empty ipv6 hostname
to avoid changing semantics for existing slirp CLI args, where the
existing ":port" exclusively means ipv4. IIC, this is also why you
needed to introduce the "is_v6" flag, because any non-empty address
can be reliably parsed without needing this flag.

This is reasonable, but any such functional change should be in a 
separate commit from refactoring.

IOW, remove this and the is_v6 flag, and add them in a separate
patch to explain to the need for new functionality in the parsing.

Given that existing callers don't need to support "[]", we should
not let that be parsed, unless the caller passing a "is_v6" pointer
which is not NULL.

> +        } else {
> +            if (sscanf(buf, "[%64[^]]]:%32s", host, port) != 2) {
> +                error_setg(errp, "error parsing IPv6 host:port '%s'", buf);
> +                return NULL;
> +            }
>          }
>      } else {
> -        /* hostname or IPv4 addr */
> -        if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, &pos) != 2) {
> -            error_setg(errp, "error parsing address '%s'", str);
> -            return -1;
> +        if (buf[0] == ':') {
> +            /* no host given */
> +            host[0] = '\0';
> +            if (sscanf(buf, ":%32s", port) != 1) {
> +                error_setg(errp, "error parsing host:port '%s'", buf);
> +                return NULL;
> +            }

It would be preferreable if the parsing code was not re-ordered when
extracting it. It doesn't look like a functional change, but I'm unsure
why you moved it ?

> +        } else {
> +            /* hostname or IPv4 addr */
> +            if (sscanf(buf, "%64[^:]:%32s", host, port) != 2) {
> +                error_setg(errp, "error parsing host:port '%s'", buf);
> +                return NULL;
> +            }
>          }
>      }
>  
> -    addr->host = g_strdup(host);
> -    addr->port = g_strdup(port);
> +    *hostp = g_strdup(host);
> +    *portp = g_strdup(port);
> +    *is_v6 = buf[0] == '[';
> +
> +    return terminator_ptr;
> +}
> +
> +int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> +{
> +    const char *optstr, *h;
> +    bool is_v6;
> +    int to;
> +    int pos;
> +    char *begin;
> +
> +    memset(addr, 0, sizeof(*addr));
> +
> +    optstr = inet_parse_host_and_port(str, ',', &addr->host, &addr->port,
> +                                      &is_v6, errp);

Just pass NULL since we don't need is_v6

> +    if (optstr == NULL) {
> +        return -1;
> +    }
>  
>      /* parse options */
> -    optstr = str + pos;
>      h = strstr(optstr, ",to=");
>      if (h) {
>          h += 4;
> -- 
> 2.30.0.617.g56c4b15f3c-goog
> 

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 v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Fri, Feb 19, 2021 at 2:00 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Feb 18, 2021 at 12:15:36PM -0800, Doug Evans wrote:
> > The parsing is moved into new function inet_parse_host_and_port.
> > This is done in preparation for using the function in net/slirp.c.
> >
> > Signed-off-by: Doug Evans <dje@google.com>
> > ---
> >
> > Changes from v3:
> > - this patch is new in v4
> >   - provides new utility: inet_parse_host_and_port, updates inet_parse
> >     to use it
> >
> >  include/qemu/sockets.h |  3 ++
> >  util/qemu-sockets.c    | 94 +++++++++++++++++++++++++++++++-----------
> >  2 files changed, 72 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > index 7d1f813576..f720378a6b 100644
> > --- a/include/qemu/sockets.h
> > +++ b/include/qemu/sockets.h
> > @@ -31,6 +31,9 @@ int socket_set_fast_reuse(int fd);
> >
> >  int inet_ai_family_from_address(InetSocketAddress *addr,
> >                                  Error **errp);
> > +const char* inet_parse_host_and_port(const char* str, int terminator,
> > +                                     char **addr, char **port, bool
> *is_v6,
> > +                                     Error **errp);
> >  int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
> >  int inet_connect(const char *str, Error **errp);
> >  int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 8af0278f15..9fca7d9212 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -615,44 +615,88 @@ static int inet_parse_flag(const char *flagname,
> const char *optstr, bool *val,
> >      return 0;
> >  }
> >
> > -int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > +/*
> > + * Parse an inet host and port as "host:port<terminator>".
> > + * Terminator may be '\0'.
> > + * The syntax for ipv4 addresses is: address:port.
> > + * The syntax for ipv6 addresses is: [address]:port.
>
> It also supports
>
>    "The syntax for hostnames is hostname:port
>
> > + * On success, returns a pointer to the terminator. Space for the
> address and
> > + * port is malloced and stored in *host, *port, the caller must free.
> > + * *is_v6 indicates whether the address is ipv4 or ipv6. If ipv6 then
> the
> > + * surrounding [] brackets are removed.
>
> When is_v6 is true, it indicates that a numeric ipv6 address was given.
> When false either a numberic ipv4 address or hostname was given.
>
> > + * On failure NULL is returned with the error stored in *errp.
> > + */
> > +const char* inet_parse_host_and_port(const char* str, int terminator,
> > +                                     char **hostp, char **portp, bool
> *is_v6,
> > +                                     Error **errp)
> >  {
> > -    const char *optstr, *h;
> > +    const char *terminator_ptr = strchr(str, terminator);
> > +    g_autofree char *buf = NULL;
> >      char host[65];
> >      char port[33];
> > -    int to;
> > -    int pos;
> > -    char *begin;
> >
> > -    memset(addr, 0, sizeof(*addr));
> > +    if (terminator_ptr == NULL) {
> > +        /* If the terminator isn't found then use the entire string. */
> > +        terminator_ptr = str + strlen(str);
> > +    }
> > +    buf = g_strndup(str, terminator_ptr - str);
> >
> > -    /* parse address */
> > -    if (str[0] == ':') {
> > -        /* no host given */
> > -        host[0] = '\0';
> > -        if (sscanf(str, ":%32[^,]%n", port, &pos) != 1) {
> > -            error_setg(errp, "error parsing port in address '%s'", str);
> > -            return -1;
> > -        }
>
>
> > -    } else if (str[0] == '[') {
> > +    if (buf[0] == '[') {
> >          /* IPv6 addr */
> > -        if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) != 2) {
> > -            error_setg(errp, "error parsing IPv6 address '%s'", str);
> > -            return -1;
> > +        if (buf[1] == ']') {
> > +            /* sscanf %[ doesn't recognize empty contents. */
> > +            host[0] = '\0';
> > +            if (sscanf(buf, "[]:%32s", port) != 1) {
> > +                error_setg(errp, "error parsing IPv6 host:port '%s'",
> buf);
> > +                return NULL;
> > +            }
>
> This is introducing new functionality to the parser. Current callers
> let empty string ":port" be used for both ipv4 and ipv6, based
> on whether the flags ",ipv4[=on|off],ipv6[=on|off]" later follow.
>


We're creating a new utility subroutine: Let's decide what the API is for
it.
The fact that inet_parse is passed additional parameters to specify ipv4 vs
ipv6 is not something this new subroutine should care about.

I presume you want an explicit way to represent an empty ipv6 hostname
> to avoid changing semantics for existing slirp CLI args, where the
> existing ":port" exclusively means ipv4. IIC, this is also why you
> needed to introduce the "is_v6" flag, because any non-empty address
> can be reliably parsed without needing this flag.
>


Actually, no. The "is_v6" flag is needed because otherwise the caller has
no means (other than maybe subsequent grepping for "." vs ":") for knowing
whether str contained "address" or "[address]".

Plus, for my needs I don't need to support "[hostname]". If someone later
wants that supported that can be designed then.
Thus supporting "[hostname]" is not something I'm considering in this
patchset.



>
> This is reasonable, but any such functional change should be in a
> separate commit from refactoring.
>
> IOW, remove this and the is_v6 flag, and add them in a separate
> patch to explain to the need for new functionality in the parsing.
>
> Given that existing callers don't need to support "[]", we should
> not let that be parsed, unless the caller passing a "is_v6" pointer
> which is not NULL.
>
> > +        } else {
> > +            if (sscanf(buf, "[%64[^]]]:%32s", host, port) != 2) {
> > +                error_setg(errp, "error parsing IPv6 host:port '%s'",
> buf);
> > +                return NULL;
> > +            }
> >          }
> >      } else {
> > -        /* hostname or IPv4 addr */
> > -        if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, &pos) != 2) {
> > -            error_setg(errp, "error parsing address '%s'", str);
> > -            return -1;
> > +        if (buf[0] == ':') {
> > +            /* no host given */
> > +            host[0] = '\0';
> > +            if (sscanf(buf, ":%32s", port) != 1) {
> > +                error_setg(errp, "error parsing host:port '%s'", buf);
> > +                return NULL;
> > +            }
>
> It would be preferreable if the parsing code was not re-ordered when
> extracting it. It doesn't look like a functional change, but I'm unsure
> why you moved it ?
>
> > +        } else {
> > +            /* hostname or IPv4 addr */
> > +            if (sscanf(buf, "%64[^:]:%32s", host, port) != 2) {
> > +                error_setg(errp, "error parsing host:port '%s'", buf);
> > +                return NULL;
> > +            }
> >          }
> >      }
> >
> > -    addr->host = g_strdup(host);
> > -    addr->port = g_strdup(port);
> > +    *hostp = g_strdup(host);
> > +    *portp = g_strdup(port);
> > +    *is_v6 = buf[0] == '[';
> > +
> > +    return terminator_ptr;
> > +}
> > +
> > +int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > +{
> > +    const char *optstr, *h;
> > +    bool is_v6;
> > +    int to;
> > +    int pos;
> > +    char *begin;
> > +
> > +    memset(addr, 0, sizeof(*addr));
> > +
> > +    optstr = inet_parse_host_and_port(str, ',', &addr->host,
> &addr->port,
> > +                                      &is_v6, errp);
>
> Just pass NULL since we don't need is_v6
>
> > +    if (optstr == NULL) {
> > +        return -1;
> > +    }
> >
> >      /* parse options */
> > -    optstr = str + pos;
> >      h = strstr(optstr, ",to=");
> >      if (h) {
> >          h += 4;
> > --
> > 2.30.0.617.g56c4b15f3c-goog
> >



I can certainly defer [] handling to a later patch series.
Splitting the patch into one with the is_v6 flag and one without is a lot
of work for little gain (zero IMO): When looking at
inet_parse_host_and_port() as its own utility subroutine, not providing the
caller with the means to distinguish whether str was "address:port" or
"[address]:port" is a poor API.
I can still revise patch to allow is_v6 being NULL though.

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Daniel P. Berrangé]

On Fri, Feb 19, 2021 at 02:17:42PM -0800, Doug Evans wrote:
> On Fri, Feb 19, 2021 at 2:00 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Thu, Feb 18, 2021 at 12:15:36PM -0800, Doug Evans wrote:
> > > The parsing is moved into new function inet_parse_host_and_port.
> > > This is done in preparation for using the function in net/slirp.c.
> > >
> > > Signed-off-by: Doug Evans <dje@google.com>
> > > ---
> > >
> > > Changes from v3:
> > > - this patch is new in v4
> > >   - provides new utility: inet_parse_host_and_port, updates inet_parse
> > >     to use it
> > >
> > >  include/qemu/sockets.h |  3 ++
> > >  util/qemu-sockets.c    | 94 +++++++++++++++++++++++++++++++-----------
> > >  2 files changed, 72 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > > index 7d1f813576..f720378a6b 100644
> > > --- a/include/qemu/sockets.h
> > > +++ b/include/qemu/sockets.h
> > > @@ -31,6 +31,9 @@ int socket_set_fast_reuse(int fd);
> > >
> > >  int inet_ai_family_from_address(InetSocketAddress *addr,
> > >                                  Error **errp);
> > > +const char* inet_parse_host_and_port(const char* str, int terminator,
> > > +                                     char **addr, char **port, bool
> > *is_v6,
> > > +                                     Error **errp);
> > >  int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
> > >  int inet_connect(const char *str, Error **errp);
> > >  int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
> > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > index 8af0278f15..9fca7d9212 100644
> > > --- a/util/qemu-sockets.c
> > > +++ b/util/qemu-sockets.c
> > > @@ -615,44 +615,88 @@ static int inet_parse_flag(const char *flagname,
> > const char *optstr, bool *val,
> > >      return 0;
> > >  }
> > >
> > > -int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > > +/*
> > > + * Parse an inet host and port as "host:port<terminator>".
> > > + * Terminator may be '\0'.
> > > + * The syntax for ipv4 addresses is: address:port.
> > > + * The syntax for ipv6 addresses is: [address]:port.
> >
> > It also supports
> >
> >    "The syntax for hostnames is hostname:port
> >
> > > + * On success, returns a pointer to the terminator. Space for the
> > address and
> > > + * port is malloced and stored in *host, *port, the caller must free.
> > > + * *is_v6 indicates whether the address is ipv4 or ipv6. If ipv6 then
> > the
> > > + * surrounding [] brackets are removed.
> >
> > When is_v6 is true, it indicates that a numeric ipv6 address was given.
> > When false either a numberic ipv4 address or hostname was given.
> >
> > > + * On failure NULL is returned with the error stored in *errp.
> > > + */
> > > +const char* inet_parse_host_and_port(const char* str, int terminator,
> > > +                                     char **hostp, char **portp, bool
> > *is_v6,
> > > +                                     Error **errp)
> > >  {
> > > -    const char *optstr, *h;
> > > +    const char *terminator_ptr = strchr(str, terminator);
> > > +    g_autofree char *buf = NULL;
> > >      char host[65];
> > >      char port[33];
> > > -    int to;
> > > -    int pos;
> > > -    char *begin;
> > >
> > > -    memset(addr, 0, sizeof(*addr));
> > > +    if (terminator_ptr == NULL) {
> > > +        /* If the terminator isn't found then use the entire string. */
> > > +        terminator_ptr = str + strlen(str);
> > > +    }
> > > +    buf = g_strndup(str, terminator_ptr - str);
> > >
> > > -    /* parse address */
> > > -    if (str[0] == ':') {
> > > -        /* no host given */
> > > -        host[0] = '\0';
> > > -        if (sscanf(str, ":%32[^,]%n", port, &pos) != 1) {
> > > -            error_setg(errp, "error parsing port in address '%s'", str);
> > > -            return -1;
> > > -        }
> >
> >
> > > -    } else if (str[0] == '[') {
> > > +    if (buf[0] == '[') {
> > >          /* IPv6 addr */
> > > -        if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) != 2) {
> > > -            error_setg(errp, "error parsing IPv6 address '%s'", str);
> > > -            return -1;
> > > +        if (buf[1] == ']') {
> > > +            /* sscanf %[ doesn't recognize empty contents. */
> > > +            host[0] = '\0';
> > > +            if (sscanf(buf, "[]:%32s", port) != 1) {
> > > +                error_setg(errp, "error parsing IPv6 host:port '%s'",
> > buf);
> > > +                return NULL;
> > > +            }
> >
> > This is introducing new functionality to the parser. Current callers
> > let empty string ":port" be used for both ipv4 and ipv6, based
> > on whether the flags ",ipv4[=on|off],ipv6[=on|off]" later follow.
> >
> 
> 
> We're creating a new utility subroutine: Let's decide what the API is for
> it.
> The fact that inet_parse is passed additional parameters to specify ipv4 vs
> ipv6 is not something this new subroutine should care about.
> 
> I presume you want an explicit way to represent an empty ipv6 hostname
> > to avoid changing semantics for existing slirp CLI args, where the
> > existing ":port" exclusively means ipv4. IIC, this is also why you
> > needed to introduce the "is_v6" flag, because any non-empty address
> > can be reliably parsed without needing this flag.
> >
> 
> 
> Actually, no. The "is_v6" flag is needed because otherwise the caller has
> no means (other than maybe subsequent grepping for "." vs ":") for knowing
> whether str contained "address" or "[address]".
> 
> Plus, for my needs I don't need to support "[hostname]". If someone later
> wants that supported that can be designed then.
> Thus supporting "[hostname]" is not something I'm considering in this
> patchset.
> 
> 
> 
> >
> > This is reasonable, but any such functional change should be in a
> > separate commit from refactoring.
> >
> > IOW, remove this and the is_v6 flag, and add them in a separate
> > patch to explain to the need for new functionality in the parsing.
> >
> > Given that existing callers don't need to support "[]", we should
> > not let that be parsed, unless the caller passing a "is_v6" pointer
> > which is not NULL.
> >
> > > +        } else {
> > > +            if (sscanf(buf, "[%64[^]]]:%32s", host, port) != 2) {
> > > +                error_setg(errp, "error parsing IPv6 host:port '%s'",
> > buf);
> > > +                return NULL;
> > > +            }
> > >          }
> > >      } else {
> > > -        /* hostname or IPv4 addr */
> > > -        if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, &pos) != 2) {
> > > -            error_setg(errp, "error parsing address '%s'", str);
> > > -            return -1;
> > > +        if (buf[0] == ':') {
> > > +            /* no host given */
> > > +            host[0] = '\0';
> > > +            if (sscanf(buf, ":%32s", port) != 1) {
> > > +                error_setg(errp, "error parsing host:port '%s'", buf);
> > > +                return NULL;
> > > +            }
> >
> > It would be preferreable if the parsing code was not re-ordered when
> > extracting it. It doesn't look like a functional change, but I'm unsure
> > why you moved it ?
> >
> > > +        } else {
> > > +            /* hostname or IPv4 addr */
> > > +            if (sscanf(buf, "%64[^:]:%32s", host, port) != 2) {
> > > +                error_setg(errp, "error parsing host:port '%s'", buf);
> > > +                return NULL;
> > > +            }
> > >          }
> > >      }
> > >
> > > -    addr->host = g_strdup(host);
> > > -    addr->port = g_strdup(port);
> > > +    *hostp = g_strdup(host);
> > > +    *portp = g_strdup(port);
> > > +    *is_v6 = buf[0] == '[';
> > > +
> > > +    return terminator_ptr;
> > > +}
> > > +
> > > +int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > > +{
> > > +    const char *optstr, *h;
> > > +    bool is_v6;
> > > +    int to;
> > > +    int pos;
> > > +    char *begin;
> > > +
> > > +    memset(addr, 0, sizeof(*addr));
> > > +
> > > +    optstr = inet_parse_host_and_port(str, ',', &addr->host,
> > &addr->port,
> > > +                                      &is_v6, errp);
> >
> > Just pass NULL since we don't need is_v6
> >
> > > +    if (optstr == NULL) {
> > > +        return -1;
> > > +    }
> > >
> > >      /* parse options */
> > > -    optstr = str + pos;
> > >      h = strstr(optstr, ",to=");
> > >      if (h) {
> > >          h += 4;
> > > --
> > > 2.30.0.617.g56c4b15f3c-goog
> > >
> 
> 
> 
> I can certainly defer [] handling to a later patch series.
> Splitting the patch into one with the is_v6 flag and one without is a lot
> of work for little gain (zero IMO): When looking at
> inet_parse_host_and_port() as its own utility subroutine, not providing the
> caller with the means to distinguish whether str was "address:port" or
> "[address]:port" is a poor API.

In general callers shouldn't care about which format was parsed. The use
of [] is just a mechanism to reliably separate the port from the address.
Once you have the address part getaddrinfo() will reliably parse the
address into a sockaddr struct on its own. The is_v6 flag is only needed
for the legacy compat needs in slirp, even that is only if we want to 
have strict equivalence with historical behaviour, as opposed to changing
empty string to mean to listen on both IPv4+6 concurrently..

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 v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Mon, Feb 22, 2021 at 1:39 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Fri, Feb 19, 2021 at 02:17:42PM -0800, Doug Evans wrote:
> > On Fri, Feb 19, 2021 at 2:00 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Thu, Feb 18, 2021 at 12:15:36PM -0800, Doug Evans wrote:
> > > > The parsing is moved into new function inet_parse_host_and_port.
> > > > This is done in preparation for using the function in net/slirp.c.
> > > >
> > > > Signed-off-by: Doug Evans <dje@google.com>
> > > > ---
> > > >
> > > > Changes from v3:
> > > > - this patch is new in v4
> > > >   - provides new utility: inet_parse_host_and_port, updates
> inet_parse
> > > >     to use it
> > > >
> > > >  include/qemu/sockets.h |  3 ++
> > > >  util/qemu-sockets.c    | 94
> +++++++++++++++++++++++++++++++-----------
> > > >  2 files changed, 72 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > > > index 7d1f813576..f720378a6b 100644
> > > > --- a/include/qemu/sockets.h
> > > > +++ b/include/qemu/sockets.h
> > > > @@ -31,6 +31,9 @@ int socket_set_fast_reuse(int fd);
> > > >
> > > >  int inet_ai_family_from_address(InetSocketAddress *addr,
> > > >                                  Error **errp);
> > > > +const char* inet_parse_host_and_port(const char* str, int
> terminator,
> > > > +                                     char **addr, char **port, bool
> > > *is_v6,
> > > > +                                     Error **errp);
> > > >  int inet_parse(InetSocketAddress *addr, const char *str, Error
> **errp);
> > > >  int inet_connect(const char *str, Error **errp);
> > > >  int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
> > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > > index 8af0278f15..9fca7d9212 100644
> > > > --- a/util/qemu-sockets.c
> > > > +++ b/util/qemu-sockets.c
> > > > @@ -615,44 +615,88 @@ static int inet_parse_flag(const char
> *flagname,
> > > const char *optstr, bool *val,
> > > >      return 0;
> > > >  }
> > > >
> > > > -int inet_parse(InetSocketAddress *addr, const char *str, Error
> **errp)
> > > > +/*
> > > > + * Parse an inet host and port as "host:port<terminator>".
> > > > + * Terminator may be '\0'.
> > > > + * The syntax for ipv4 addresses is: address:port.
> > > > + * The syntax for ipv6 addresses is: [address]:port.
> > >
> > > It also supports
> > >
> > >    "The syntax for hostnames is hostname:port
> > >
> > > > + * On success, returns a pointer to the terminator. Space for the
> > > address and
> > > > + * port is malloced and stored in *host, *port, the caller must
> free.
> > > > + * *is_v6 indicates whether the address is ipv4 or ipv6. If ipv6
> then
> > > the
> > > > + * surrounding [] brackets are removed.
> > >
> > > When is_v6 is true, it indicates that a numeric ipv6 address was given.
> > > When false either a numberic ipv4 address or hostname was given.
> > >
> > > > + * On failure NULL is returned with the error stored in *errp.
> > > > + */
> > > > +const char* inet_parse_host_and_port(const char* str, int
> terminator,
> > > > +                                     char **hostp, char **portp,
> bool
> > > *is_v6,
> > > > +                                     Error **errp)
> > > >  {
> > > > -    const char *optstr, *h;
> > > > +    const char *terminator_ptr = strchr(str, terminator);
> > > > +    g_autofree char *buf = NULL;
> > > >      char host[65];
> > > >      char port[33];
> > > > -    int to;
> > > > -    int pos;
> > > > -    char *begin;
> > > >
> > > > -    memset(addr, 0, sizeof(*addr));
> > > > +    if (terminator_ptr == NULL) {
> > > > +        /* If the terminator isn't found then use the entire
> string. */
> > > > +        terminator_ptr = str + strlen(str);
> > > > +    }
> > > > +    buf = g_strndup(str, terminator_ptr - str);
> > > >
> > > > -    /* parse address */
> > > > -    if (str[0] == ':') {
> > > > -        /* no host given */
> > > > -        host[0] = '\0';
> > > > -        if (sscanf(str, ":%32[^,]%n", port, &pos) != 1) {
> > > > -            error_setg(errp, "error parsing port in address '%s'",
> str);
> > > > -            return -1;
> > > > -        }
> > >
> > >
> > > > -    } else if (str[0] == '[') {
> > > > +    if (buf[0] == '[') {
> > > >          /* IPv6 addr */
> > > > -        if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) !=
> 2) {
> > > > -            error_setg(errp, "error parsing IPv6 address '%s'",
> str);
> > > > -            return -1;
> > > > +        if (buf[1] == ']') {
> > > > +            /* sscanf %[ doesn't recognize empty contents. */
> > > > +            host[0] = '\0';
> > > > +            if (sscanf(buf, "[]:%32s", port) != 1) {
> > > > +                error_setg(errp, "error parsing IPv6 host:port
> '%s'",
> > > buf);
> > > > +                return NULL;
> > > > +            }
> > >
> > > This is introducing new functionality to the parser. Current callers
> > > let empty string ":port" be used for both ipv4 and ipv6, based
> > > on whether the flags ",ipv4[=on|off],ipv6[=on|off]" later follow.
> > >
> >
> >
> > We're creating a new utility subroutine: Let's decide what the API is for
> > it.
> > The fact that inet_parse is passed additional parameters to specify ipv4
> vs
> > ipv6 is not something this new subroutine should care about.
> >
> > I presume you want an explicit way to represent an empty ipv6 hostname
> > > to avoid changing semantics for existing slirp CLI args, where the
> > > existing ":port" exclusively means ipv4. IIC, this is also why you
> > > needed to introduce the "is_v6" flag, because any non-empty address
> > > can be reliably parsed without needing this flag.
> > >
> >
> >
> > Actually, no. The "is_v6" flag is needed because otherwise the caller has
> > no means (other than maybe subsequent grepping for "." vs ":") for
> knowing
> > whether str contained "address" or "[address]".
> >
> > Plus, for my needs I don't need to support "[hostname]". If someone later
> > wants that supported that can be designed then.
> > Thus supporting "[hostname]" is not something I'm considering in this
> > patchset.
> >
> >
> >
> > >
> > > This is reasonable, but any such functional change should be in a
> > > separate commit from refactoring.
> > >
> > > IOW, remove this and the is_v6 flag, and add them in a separate
> > > patch to explain to the need for new functionality in the parsing.
> > >
> > > Given that existing callers don't need to support "[]", we should
> > > not let that be parsed, unless the caller passing a "is_v6" pointer
> > > which is not NULL.
> > >
> > > > +        } else {
> > > > +            if (sscanf(buf, "[%64[^]]]:%32s", host, port) != 2) {
> > > > +                error_setg(errp, "error parsing IPv6 host:port
> '%s'",
> > > buf);
> > > > +                return NULL;
> > > > +            }
> > > >          }
> > > >      } else {
> > > > -        /* hostname or IPv4 addr */
> > > > -        if (sscanf(str, "%64[^:]:%32[^,]%n", host, port, &pos) !=
> 2) {
> > > > -            error_setg(errp, "error parsing address '%s'", str);
> > > > -            return -1;
> > > > +        if (buf[0] == ':') {
> > > > +            /* no host given */
> > > > +            host[0] = '\0';
> > > > +            if (sscanf(buf, ":%32s", port) != 1) {
> > > > +                error_setg(errp, "error parsing host:port '%s'",
> buf);
> > > > +                return NULL;
> > > > +            }
> > >
> > > It would be preferreable if the parsing code was not re-ordered when
> > > extracting it. It doesn't look like a functional change, but I'm unsure
> > > why you moved it ?
> > >
> > > > +        } else {
> > > > +            /* hostname or IPv4 addr */
> > > > +            if (sscanf(buf, "%64[^:]:%32s", host, port) != 2) {
> > > > +                error_setg(errp, "error parsing host:port '%s'",
> buf);
> > > > +                return NULL;
> > > > +            }
> > > >          }
> > > >      }
> > > >
> > > > -    addr->host = g_strdup(host);
> > > > -    addr->port = g_strdup(port);
> > > > +    *hostp = g_strdup(host);
> > > > +    *portp = g_strdup(port);
> > > > +    *is_v6 = buf[0] == '[';
> > > > +
> > > > +    return terminator_ptr;
> > > > +}
> > > > +
> > > > +int inet_parse(InetSocketAddress *addr, const char *str, Error
> **errp)
> > > > +{
> > > > +    const char *optstr, *h;
> > > > +    bool is_v6;
> > > > +    int to;
> > > > +    int pos;
> > > > +    char *begin;
> > > > +
> > > > +    memset(addr, 0, sizeof(*addr));
> > > > +
> > > > +    optstr = inet_parse_host_and_port(str, ',', &addr->host,
> > > &addr->port,
> > > > +                                      &is_v6, errp);
> > >
> > > Just pass NULL since we don't need is_v6
> > >
> > > > +    if (optstr == NULL) {
> > > > +        return -1;
> > > > +    }
> > > >
> > > >      /* parse options */
> > > > -    optstr = str + pos;
> > > >      h = strstr(optstr, ",to=");
> > > >      if (h) {
> > > >          h += 4;
> > > > --
> > > > 2.30.0.617.g56c4b15f3c-goog
> > > >
> >
> >
> >
> > I can certainly defer [] handling to a later patch series.
> > Splitting the patch into one with the is_v6 flag and one without is a lot
> > of work for little gain (zero IMO): When looking at
> > inet_parse_host_and_port() as its own utility subroutine, not providing
> the
> > caller with the means to distinguish whether str was "address:port" or
> > "[address]:port" is a poor API.
>
> In general callers shouldn't care about which format was parsed. The use
> of [] is just a mechanism to reliably separate the port from the address.
> Once you have the address part getaddrinfo() will reliably parse the
> address into a sockaddr struct on its own. The is_v6 flag is only needed
> for the legacy compat needs in slirp, even that is only if we want to
> have strict equivalence with historical behaviour, as opposed to changing
> empty string to mean to listen on both IPv4+6 concurrently..
>


I guess I'll wait for Samuel to review the net/slirp changes. No point in
continually sending revisions until then.

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Hello,

Daniel P. Berrangé, le lun. 22 févr. 2021 09:39:41 +0000, a ecrit:
> In general callers shouldn't care about which format was parsed. The use
> of [] is just a mechanism to reliably separate the port from the address.
> Once you have the address part getaddrinfo() will reliably parse the
> address into a sockaddr struct on its own.

Agreed.

> The is_v6 flag is only needed
> for the legacy compat needs in slirp, even that is only if we want to 
> have strict equivalence with historical behaviour, as opposed to changing
> empty string to mean to listen on both IPv4+6 concurrently..

I would say that empty address meaning ipv4+6 looks better to me.

Doug Evans, le lun. 22 févr. 2021 09:55:09 -0800, a ecrit:
> Hi guys. I think before I submit yet another patchset in this series I need
> someone with authority to define the user API for ipv6 host forwarding.
> Since the hostfwd syntax is parsed in net/slirp.c, Samuel I think that means
> you (based on what I'm reading in MAINTAINERS).

Well, I'm not maintainer of the user API actually. That'd rather be
Markus Armbruster, now Cc-ed, who devises the command-line options,
QAPI, etc.

> Based on what Maxim originally wrote I was going with addresses wrapped in []
> mean ipv6, but Daniel does not want that.

Specifying [127.0.0.1] would be odd, but for instance 

ssh localhost -D '[127.0.0.1]':23456

happens to listen on 127.0.0.1. So I would say that common practice
really is that [] only matters for syntax, and not semantic.

> There are issues to consider of course.
> Note that one issue I am leaving for later (i.e., I don't want to drag this
> patch series out to include it), is whether and how to support ipv4-host->
> ipv6-guest forwarding and vice versa. Can libslirp support this?

That would be feasible yes: since the data flow is completely rebuilt
between the host and the guest, there is no remnant of the IP version.
It was simpler to have e.g. udp_listen and udp6_listen separate to keep
uint32_t / in6_addr parameters, but there is no strict reason for this:
the haddr is only passed to the bind() call, and the laddr is only
recorded in the so. Put another way, a refactoring patch could be to
just hand udp_listen two sockaddrs, and it will just work fine. We'd
then introduce a slirp_add_hostfwd that takes two sockaddr instead of
host/port.

> Setting cross-forwarding aside, we can't break existing uses of course, so that
> means that one issue is that if [] is not used to identify ipv6 addresses then
> something like ",ipv6" will be needed as a separate argument; otherwise the
> empty address "" will be ambiguous.

(as I mentioned above, I'd say empty address "" should mean ipv4+ipv6)

> +  Examples:
> +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22

Yep, that looks good to me.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Samuel Thibault, le dim. 28 févr. 2021 22:39:57 +0100, a ecrit:
> It was simpler to have e.g. udp_listen and udp6_listen separate to keep
> uint32_t / in6_addr parameters, but there is no strict reason for this:
> the haddr is only passed to the bind() call, and the laddr is only
> recorded in the so. Put another way, a refactoring patch could be to
> just hand udp_listen two sockaddrs, and it will just work fine.

I have submitted that part to
https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/74
Could you review it?

> We'd then introduce a slirp_add_xhostfwd that takes two sockaddr
> instead of host/port.

That should then be easy to introduce indeed, and immediately more
powerful than the slirp_add/remove_ipv6_hostfwd. Possibly you could just
replace in qemu the existing slirp_add/remove_hostfwd call, and thus
make the whole code simpler: ideally it's the address parsing function
that would produce a sockaddr.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Markus Armbruster]

Samuel Thibault <samuel.thibault@gnu.org> writes:

> Hello,
>
> Daniel P. Berrangé, le lun. 22 févr. 2021 09:39:41 +0000, a ecrit:
>> In general callers shouldn't care about which format was parsed. The use
>> of [] is just a mechanism to reliably separate the port from the address.
>> Once you have the address part getaddrinfo() will reliably parse the
>> address into a sockaddr struct on its own.
>
> Agreed.
>
>> The is_v6 flag is only needed
>> for the legacy compat needs in slirp, even that is only if we want to 
>> have strict equivalence with historical behaviour, as opposed to changing
>> empty string to mean to listen on both IPv4+6 concurrently..
>
> I would say that empty address meaning ipv4+6 looks better to me.
>
> Doug Evans, le lun. 22 févr. 2021 09:55:09 -0800, a ecrit:
>> Hi guys. I think before I submit yet another patchset in this series I need
>> someone with authority to define the user API for ipv6 host forwarding.
>> Since the hostfwd syntax is parsed in net/slirp.c, Samuel I think that means
>> you (based on what I'm reading in MAINTAINERS).
>
> Well, I'm not maintainer of the user API actually. That'd rather be
> Markus Armbruster, now Cc-ed, who devises the command-line options,
> QAPI, etc.

I rarely devise, I just try to keep things sane by reviewing and
advising, with the help of others.

>> Based on what Maxim originally wrote I was going with addresses wrapped in []
>> mean ipv6, but Daniel does not want that.
>
> Specifying [127.0.0.1] would be odd, but for instance 
>
> ssh localhost -D '[127.0.0.1]':23456
>
> happens to listen on 127.0.0.1. So I would say that common practice
> really is that [] only matters for syntax, and not semantic.

I believe common syntactic practice is to use [brackets] only around
numeric IPv6 addresses.  E.g. socat(1):

       IP address
              An IPv4 address in numbers-and-dots notation, an IPv6 address in
              hex notation enclosed in brackets, or a hostname  that  resolves
              to an IPv4 or an IPv6 address.
              Examples: 127.0.0.1, [::1], www.dest-unreach.org, dns1

[...]

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Markus Armbruster, le lun. 01 mars 2021 09:15:41 +0100, a ecrit:
> Samuel Thibault <samuel.thibault@gnu.org> writes:
> > Specifying [127.0.0.1] would be odd, but for instance 
> >
> > ssh localhost -D '[127.0.0.1]':23456
> >
> > happens to listen on 127.0.0.1. So I would say that common practice
> > really is that [] only matters for syntax, and not semantic.
> 
> I believe common syntactic practice is to use [brackets] only around
> numeric IPv6 addresses.  E.g. socat(1):
> 
>        IP address
>               An IPv4 address in numbers-and-dots notation, an IPv6 address in
>               hex notation enclosed in brackets, or a hostname  that  resolves
>               to an IPv4 or an IPv6 address.
>               Examples: 127.0.0.1, [::1], www.dest-unreach.org, dns1

Yes and that's also what ssh documents, but in ssh the brackets happen
to also work for an IPv4 address.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Hello,
>
> Daniel P. Berrangé, le lun. 22 févr. 2021 09:39:41 +0000, a ecrit:
>
> > The is_v6 flag is only needed
> > for the legacy compat needs in slirp, even that is only if we want to
> > have strict equivalence with historical behaviour, as opposed to changing
> > empty string to mean to listen on both IPv4+6 concurrently..
>
> I would say that empty address meaning ipv4+6 looks better to me.
>


It's not my call, but I have some questions.

Are there any users that this functional change would break?
[Previously the empty address meant qemu would only listen on ipv4
addr-any.]

What if a user wants only ipv4 addr-any (or only ipv6 addr-any) ?

What does hostfwd "::12345-6.7.8.9:10" mean?
Does the presence of the empty host address mean forward both ipv4 and ipv6
to guest ipv4 6.7.8.9?

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Doug Evans, le lun. 01 mars 2021 08:07:19 -0800, a ecrit:
> Are there any users that this functional change would break?
> [Previously the empty address meant qemu would only listen on ipv4 addr-any.]

One case that could be broken would be a user having already another
service listening on ipv6-only along qemu listening on ipv4-only. But I
find this very little probable.

> What if a user wants only ipv4 addr-any (or only ipv6 addr-any) ?

"0.0.0.0" would get ipv4 addr-any.

Without anything done in particular, "::" would get both ipv6 and
ipv4. We could make libslirp enable the IPV6ONLY flag to avoid that, and
make qemu pass an AF_UNSPEC address for the ipv4+ipv6 case, in which
case libslirp wouldn't set IPV6ONLY.

make that ipv6-only through a flag passed to 

> What does hostfwd "::12345-6.7.8.9:10" mean?
> Does the presence of the empty host address mean forward both ipv4 and ipv6 to
> guest ipv4 6.7.8.9?

I'd say so, yes.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Samuel Thibault, le lun. 01 mars 2021 17:26:23 +0100, a ecrit:
> We could make libslirp enable the IPV6ONLY flag to avoid that, and
> make qemu pass an AF_UNSPEC address for the ipv4+ipv6 case, in which
> case libslirp wouldn't set IPV6ONLY.

Ah, no, AF_UNSPEC would not allow to specify the port. But we can use a
flag in the slirp_add_hostxfwd call.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> [...]
> > Note that one issue I am leaving for later (i.e., I don't want to drag
> this
> > patch series out to include it), is whether and how to support
> ipv4-host->
> > ipv6-guest forwarding and vice versa. Can libslirp support this?
>
> That would be feasible yes: since the data flow is completely rebuilt
> between the host and the guest, there is no remnant of the IP version.
> It was simpler to have e.g. udp_listen and udp6_listen separate to keep
> uint32_t / in6_addr parameters, but there is no strict reason for this:
> the haddr is only passed to the bind() call, and the laddr is only
> recorded in the so. Put another way, a refactoring patch could be to
> just hand udp_listen two sockaddrs, and it will just work fine. We'd
> then introduce a slirp_add_hostfwd that takes two sockaddr instead of
> host/port.
>


I guess I'm not familiar enough with this code.
Help me understand how passing two addresses to udp_listen is simpler.
That feels confusing from an API viewpoint.

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Doug Evans, le lun. 01 mars 2021 08:23:03 -0800, a ecrit:
> On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <[1]samuel.thibault@gnu.org>
> wrote:
> 
>     [...]
>     > Note that one issue I am leaving for later (i.e., I don't want to drag
>     this
>     > patch series out to include it), is whether and how to support
>     ipv4-host->
>     > ipv6-guest forwarding and vice versa. Can libslirp support this?
> 
>     That would be feasible yes: since the data flow is completely rebuilt
>     between the host and the guest, there is no remnant of the IP version.
>     It was simpler to have e.g. udp_listen and udp6_listen separate to keep
>     uint32_t / in6_addr parameters, but there is no strict reason for this:
>     the haddr is only passed to the bind() call, and the laddr is only
>     recorded in the so. Put another way, a refactoring patch could be to
>     just hand udp_listen two sockaddrs, and it will just work fine. We'd
>     then introduce a slirp_add_hostfwd that takes two sockaddr instead of
>     host/port.
> 
> 
> 
> I guess I'm not familiar enough with this code.
> Help me understand how passing two addresses to udp_listen is simpler.
> That feels confusing from an API viewpoint.

? udp_listen already takes two addresses + two ports. I just mean
replacing that with two sockaddr, which contains both, but also contains
the address family. I submitted this to 

https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/74

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Samuel Thibault, le lun. 01 mars 2021 17:27:47 +0100, a ecrit:
> Doug Evans, le lun. 01 mars 2021 08:23:03 -0800, a ecrit:
> > On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <[1]samuel.thibault@gnu.org>
> > wrote:
> > 
> >     [...]
> >     > Note that one issue I am leaving for later (i.e., I don't want to drag
> >     this
> >     > patch series out to include it), is whether and how to support
> >     ipv4-host->
> >     > ipv6-guest forwarding and vice versa. Can libslirp support this?
> > 
> >     That would be feasible yes: since the data flow is completely rebuilt
> >     between the host and the guest, there is no remnant of the IP version.
> >     It was simpler to have e.g. udp_listen and udp6_listen separate to keep
> >     uint32_t / in6_addr parameters, but there is no strict reason for this:
> >     the haddr is only passed to the bind() call, and the laddr is only
> >     recorded in the so. Put another way, a refactoring patch could be to
> >     just hand udp_listen two sockaddrs, and it will just work fine. We'd
> >     then introduce a slirp_add_hostfwd that takes two sockaddr instead of
> >     host/port.
> > 
> > 
> > 
> > I guess I'm not familiar enough with this code.
> > Help me understand how passing two addresses to udp_listen is simpler.
> > That feels confusing from an API viewpoint.
> 
> ? udp_listen already takes two addresses + two ports. I just mean
> replacing that with two sockaddr, which contains both, but also contains
> the address family. I submitted this to 
> 
> https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/74

And the public API to 
https://gitlab.freedesktop.org/slirp/libslirp/-/merge_requests/75

Ideally we'd then just drop the ipv6-only public API.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> [...]
>
> > +  Examples:
> > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
>
> Yep, that looks good to me.
>
>

Daniel, you wanted me to use inet_parse().
Is the above syntax ok with you?
You must have had some expectation that at least some of
the various flags that inet_parse() recognizes would be needed here.
Can you elaborate?

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Daniel P. Berrangé]

On Wed, Mar 03, 2021 at 10:06:50AM -0800, Doug Evans wrote:
> On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <samuel.thibault@gnu.org>
> wrote:
> 
> > [...]
> >
> > > +  Examples:
> > > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> > > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
> >
> > Yep, that looks good to me.
> >
> >
> 
> Daniel, you wanted me to use inet_parse().
> Is the above syntax ok with you?
> You must have had some expectation that at least some of
> the various flags that inet_parse() recognizes would be needed here.

It feels like the ,ipv4=on|off,ipv6=on|off flags are relevant here,
especially in the empty address case. eg

   tcp::10022          - attempt to listen on both ipv4 + ipv6
   tcp::10022,ipv4=off - listen on default address, but only for ipv6
   tcp::10022,ipv6=off - listen on default address, but only for ipv4

Basically this ends up bringing the hostfwd stuff into alignment with
the way other backends deal with this

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 v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Daniel P. Berrangé, le mer. 03 mars 2021 18:11:41 +0000, a ecrit:
> On Wed, Mar 03, 2021 at 10:06:50AM -0800, Doug Evans wrote:
> > On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <samuel.thibault@gnu.org>
> > wrote:
> > 
> > > > +  Examples:
> > > > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> > > > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
> > >
> > > Yep, that looks good to me.
> > 
> > Daniel, you wanted me to use inet_parse().
> > Is the above syntax ok with you?
> > You must have had some expectation that at least some of
> > the various flags that inet_parse() recognizes would be needed here.
> 
> It feels like the ,ipv4=on|off,ipv6=on|off flags are relevant here,
> especially in the empty address case. eg
> 
>    tcp::10022          - attempt to listen on both ipv4 + ipv6
>    tcp::10022,ipv4=off - listen on default address, but only for ipv6
>    tcp::10022,ipv6=off - listen on default address, but only for ipv4
> 
> Basically this ends up bringing the hostfwd stuff into alignment with
> the way other backends deal with this

I'm fine with this yes, better have a coherent user interface.

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Fri, Mar 5, 2021 at 1:28 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Daniel P. Berrangé, le mer. 03 mars 2021 18:11:41 +0000, a ecrit:
> > On Wed, Mar 03, 2021 at 10:06:50AM -0800, Doug Evans wrote:
> > > On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <
> samuel.thibault@gnu.org>
> > > wrote:
> > >
> > > > > +  Examples:
> > > > > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> > > > > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
> > > >
> > > > Yep, that looks good to me.
> > >
> > > Daniel, you wanted me to use inet_parse().
> > > Is the above syntax ok with you?
> > > You must have had some expectation that at least some of
> > > the various flags that inet_parse() recognizes would be needed here.
> >
> > It feels like the ,ipv4=on|off,ipv6=on|off flags are relevant here,
> > especially in the empty address case. eg
> >
> >    tcp::10022          - attempt to listen on both ipv4 + ipv6
> >    tcp::10022,ipv4=off - listen on default address, but only for ipv6
> >    tcp::10022,ipv6=off - listen on default address, but only for ipv4
> >
> > Basically this ends up bringing the hostfwd stuff into alignment with
> > the way other backends deal with this
>
> I'm fine with this yes, better have a coherent user interface.
>

Cool. Here's the current help text I have:

---snip---
#ifdef CONFIG_SLIRP
    {
        .name       = "hostfwd_add",
        .args_type  = "arg1:s,arg2:s?",
        .params     = "[netdev_id]
[tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport",
        .help       = "redirect TCP or UDP connections from host to guest
(requires -net user)",
        .cmd        = hmp_hostfwd_add,
    },
#endif
SRST
``hostfwd_add``
  Redirect TCP or UDP connections from host to guest (requires -net user).
  IPv6 addresses are wrapped in square brackets, IPv4 addresses are not.

  Examples:
  hostfwd_add net0 tcp:127.0.0.1:10022-:22
  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22

  Empty host addresses:
  An empty address for the host, expressed as either "" or "[]", is
  interpreted as listen at any address for both IPv4 and IPv6. To listen on
  only IPv4 one can use "0.0.0.0". The equivalent IPv6 address, "[::]", is
  interpreted as listen on both IPv4 and IPv6 addresses. To listen on only
  IPv6 addresses, add ",ipv4=off" to the address. E.g.,
  hostfwd_add net0 tcp::10022,ipv4=off-[fe80::1:2:3:4]:22
  hostfwd_add net0 tcp:[]:10022,ipv4=off-[fe80::1:2:3:4]:22
  hostfwd_add net0 tcp:[::]:10022,ipv4=off-[fe80::1:2:3:4]:22
  And similarly for IPv4 only:
  hostfwd_add net0 tcp::10022,ipv6=off-[fe80::1:2:3:4]:22
  hostfwd_add net0 tcp:[]:10022,ipv6=off-[fe80::1:2:3:4]:22

  Empty guest addresses:
  An empty guest address for IPv4 is translated to the guest's address,
  assuming that the guest is using DHCP to acquire its address.
  Note that Libslirp currently only provides a "stateless" DHCPv6 server, a
  consequence of which is that it cannot do the "addr-any" translation to
the
  guest address that is done for IPv4. In other words, the following is
  currently not supported: hostfwd_add net0 tcp::10022-:22, the guest
  address is required.
ERST
---snip---

Any corrections or suggestions?

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Fri, Mar 5, 2021 at 1:51 PM Doug Evans <dje@google.com> wrote:

> On Fri, Mar 5, 2021 at 1:28 PM Samuel Thibault <samuel.thibault@gnu.org>
> wrote:
>
>> Daniel P. Berrangé, le mer. 03 mars 2021 18:11:41 +0000, a ecrit:
>> > On Wed, Mar 03, 2021 at 10:06:50AM -0800, Doug Evans wrote:
>> > > On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <
>> samuel.thibault@gnu.org>
>> > > wrote:
>> > >
>> > > > > +  Examples:
>> > > > > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
>> > > > > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
>> > > >
>> > > > Yep, that looks good to me.
>> > >
>> > > Daniel, you wanted me to use inet_parse().
>> > > Is the above syntax ok with you?
>> > > You must have had some expectation that at least some of
>> > > the various flags that inet_parse() recognizes would be needed here.
>> >
>> > It feels like the ,ipv4=on|off,ipv6=on|off flags are relevant here,
>> > especially in the empty address case. eg
>> >
>> >    tcp::10022          - attempt to listen on both ipv4 + ipv6
>> >    tcp::10022,ipv4=off - listen on default address, but only for ipv6
>> >    tcp::10022,ipv6=off - listen on default address, but only for ipv4
>> >
>> > Basically this ends up bringing the hostfwd stuff into alignment with
>> > the way other backends deal with this
>>
>> I'm fine with this yes, better have a coherent user interface.
>>
>
> Cool. Here's the current help text I have:
>
> ---snip---
> #ifdef CONFIG_SLIRP
>     {
>         .name       = "hostfwd_add",
>         .args_type  = "arg1:s,arg2:s?",
>         .params     = "[netdev_id]
> [tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport",
>         .help       = "redirect TCP or UDP connections from host to guest
> (requires -net user)",
>         .cmd        = hmp_hostfwd_add,
>     },
> #endif
> SRST
> ``hostfwd_add``
>   Redirect TCP or UDP connections from host to guest (requires -net user).
>   IPv6 addresses are wrapped in square brackets, IPv4 addresses are not.
>
>   Examples:
>   hostfwd_add net0 tcp:127.0.0.1:10022-:22
>   hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
>
>   Empty host addresses:
>   An empty address for the host, expressed as either "" or "[]", is
>   interpreted as listen at any address for both IPv4 and IPv6. To listen on
>   only IPv4 one can use "0.0.0.0". The equivalent IPv6 address, "[::]", is
>   interpreted as listen on both IPv4 and IPv6 addresses. To listen on only
>   IPv6 addresses, add ",ipv4=off" to the address. E.g.,
>   hostfwd_add net0 tcp::10022,ipv4=off-[fe80::1:2:3:4]:22
>   hostfwd_add net0 tcp:[]:10022,ipv4=off-[fe80::1:2:3:4]:22
>   hostfwd_add net0 tcp:[::]:10022,ipv4=off-[fe80::1:2:3:4]:22
>   And similarly for IPv4 only:
>   hostfwd_add net0 tcp::10022,ipv6=off-[fe80::1:2:3:4]:22
>   hostfwd_add net0 tcp:[]:10022,ipv6=off-[fe80::1:2:3:4]:22
>
>   Empty guest addresses:
>   An empty guest address for IPv4 is translated to the guest's address,
>   assuming that the guest is using DHCP to acquire its address.
>   Note that Libslirp currently only provides a "stateless" DHCPv6 server, a
>   consequence of which is that it cannot do the "addr-any" translation to
> the
>   guest address that is done for IPv4. In other words, the following is
>   currently not supported: hostfwd_add net0 tcp::10022-:22, the guest
>   address is required.
> ERST
> ---snip---
>
> Any corrections or suggestions?
>


For those following along, there are problems with the above help text
(e.g., it's wrong to say "tcp::10022-:22" is not supported, because it
obviously is! - the issue is more nuanced than that).
And I'm sure there are more that I have yet to find.
I'll give reviewers some time to comment on what's there now
before sending an updated proposed text.

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Fri, Mar 5, 2021 at 1:28 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Daniel P. Berrangé, le mer. 03 mars 2021 18:11:41 +0000, a ecrit:
> > On Wed, Mar 03, 2021 at 10:06:50AM -0800, Doug Evans wrote:
> > > On Sun, Feb 28, 2021 at 1:40 PM Samuel Thibault <
> samuel.thibault@gnu.org>
> > > wrote:
> > >
> > > > > +  Examples:
> > > > > +  hostfwd_add net0 tcp:127.0.0.1:10022-:22
> > > > > +  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
> > > >
> > > > Yep, that looks good to me.
> > >
> > > Daniel, you wanted me to use inet_parse().
> > > Is the above syntax ok with you?
> > > You must have had some expectation that at least some of
> > > the various flags that inet_parse() recognizes would be needed here.
> >
> > It feels like the ,ipv4=on|off,ipv6=on|off flags are relevant here,
> > especially in the empty address case. eg
> >
> >    tcp::10022          - attempt to listen on both ipv4 + ipv6
> >    tcp::10022,ipv4=off - listen on default address, but only for ipv6
> >    tcp::10022,ipv6=off - listen on default address, but only for ipv4
> >
> > Basically this ends up bringing the hostfwd stuff into alignment with
> > the way other backends deal with this
>
> I'm fine with this yes, better have a coherent user interface.
>


Hi. Questions regarding an empty *guest* address, e.g., either of
tcp::10022-:22
tcp::10022-[]:22

Given that the code is not supposed to be able to know brackets were present
(they're stripped off early on), what does the above mean w.r.t. the guest?
For the host we can have "" mean listen on both IPv4 and IPv6
(by default, absent extra options like ipv4=off).
But what does a guest address of "" mean?
IPv4? IPv6? Both?
Does an empty guest address take on the IPvN of the host's spec?

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Doug Evans, le ven. 05 mars 2021 16:05:05 -0800, a ecrit:
> Given that the code is not supposed to be able to know brackets were present
> (they're stripped off early on), what does the above mean w.r.t. the guest?
> For the host we can have "" mean listen on both IPv4 and IPv6
> (by default, absent extra options like ipv4=off).
> But what does a guest address of "" mean?
> IPv4? IPv6? Both?

It cannot really be "both" since it'd need to know.

The 0.0.0.0 address used to mean the dhcp-provided address, and we don't
have a way to know the ipv6 address used with the stateless selection,
so I would say that empty address would be just the dhcp-provided
address. Most probably the guest will pick it up anyway, and guests
usually listen the same on ipv4 and ipv6, so I'd say empty most probably
means the user wants to just connect to ipv4 (whatever protocol was used
to connect to the host).

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Fri, Mar 5, 2021 at 4:10 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Doug Evans, le ven. 05 mars 2021 16:05:05 -0800, a ecrit:
> > Given that the code is not supposed to be able to know brackets were
> present
> > (they're stripped off early on), what does the above mean w.r.t. the
> guest?
> > For the host we can have "" mean listen on both IPv4 and IPv6
> > (by default, absent extra options like ipv4=off).
> > But what does a guest address of "" mean?
> > IPv4? IPv6? Both?
>
> It cannot really be "both" since it'd need to know.
>
> The 0.0.0.0 address used to mean the dhcp-provided address, and we don't
> have a way to know the ipv6 address used with the stateless selection,
> so I would say that empty address would be just the dhcp-provided
> address. Most probably the guest will pick it up anyway, and guests
> usually listen the same on ipv4 and ipv6, so I'd say empty most probably
> means the user wants to just connect to ipv4 (whatever protocol was used
> to connect to the host).
>


I realize IPv6 obviates the need for a stateful DHCPv6 server.
Alas setting the hostfwd on the command line is *nice*, but it is currently
*impossible* for IPv6: many system's macaddrs are random,
and their IPv6 address is (at least often) derived from their macaddr.
Thus for those systems the hostfwds have to be set up after the guest has
booted enough to announce its address, and then the user can obtain the
address to pass to hostfwd_add either by logging in and running
ifconfig or some such (which can't be done
via ssh from the host with user-mode networking because the hostfwd
doesn't exist yet), or querying the NDP table and hope it's there.
[I'm probably missing a better alternative though, and just haven't come
up with it yet. Is it possible for QEMU to lazily determine the
guest's IPv6 address? I.e., postpone the ""->guest address mapping
until it's needed and then, say, take the first entry in the NDP table?
That feels a bit fragile: what if someone else gets the first entry in
the NDP table? But is that any more fragile than assuming the first
handed out DHCP address is to the guest?  [<<-- Honest question,
can we assume the first handed out DHCP address will necessarily
be the guest?]

Anyways,
If we eventually want a way to say "take this place-holder address
and map it to the guest's IPv6 address" and follow the current spec,
the preferable syntax would be ",ipv4" or ",ipv6" (fortunately that works -
using
",ipv6=off" or ",ipv4=off" is pretty clumsy). And then we'll have to of
course
flag ",ipv4=off,ipv6=off" and ",ipv4=on,ipv6=on" as errors.
But that would mean the defaults for the guest would have to be
different than for the host. E.g.,
host: ",ipv4" means both, whereas
guest: ",ipv4" (ideally) means ipv4 (since both is meaningless)

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

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Samuel Thibault]

Hello,

Doug Evans, le ven. 05 mars 2021 17:00:13 -0800, a ecrit:
> Is it possible for QEMU to lazily determine the guest's IPv6
> address? I.e., postpone the ""->guest address mapping until it's
> needed and then, say, take the first entry in the NDP table?

That would probably be possible, yes, by moving the 

if (!guest_addr.s_addr) {
    guest_addr = slirp->vdhcp_startaddr;
}

from slirp_add_hostfwd() and alike to tcp_connect() and sorecvfrom()
(along the other sotranslate call).

> That feels a bit fragile: what if someone else gets the first entry in
> the NDP table? But is that any more fragile than assuming the first
> handed out DHCP address is to the guest?

I don't think it's really more fragile.

> [<<-- Honest question, can we assume the first handed out DHCP address
> will necessarily be the guest?]

It "cannot" be anything else. What could happen is a PXE loader that
uses DHCP/NDP, and then the OS that does it again.

> But that would mean the defaults for the guest would have to be
> different than for the host. E.g.,
> host: ",ipv4" means both,

Why would it mean both? I don't follow you here.

> whereas guest: ",ipv4" (ideally) means ipv4 (since both is meaningless)

Samuel

Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse

[Doug Evans]

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

On Sat, Mar 6, 2021 at 11:29 AM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Hello,
>
> Doug Evans, le ven. 05 mars 2021 17:00:13 -0800, a ecrit:
> > Is it possible for QEMU to lazily determine the guest's IPv6
> > address? I.e., postpone the ""->guest address mapping until it's
> > needed and then, say, take the first entry in the NDP table?
>
> That would probably be possible, yes, by moving the
>
> if (!guest_addr.s_addr) {
>     guest_addr = slirp->vdhcp_startaddr;
> }
>
> from slirp_add_hostfwd() and alike to tcp_connect() and sorecvfrom()
> (along the other sotranslate call).
>
> > That feels a bit fragile: what if someone else gets the first entry in
> > the NDP table? But is that any more fragile than assuming the first
> > handed out DHCP address is to the guest?
>
> I don't think it's really more fragile.
>


Good to know, thanks.


> > [<<-- Honest question, can we assume the first handed out DHCP address
> > will necessarily be the guest?]
>
> It "cannot" be anything else. What could happen is a PXE loader that
> uses DHCP/NDP, and then the OS that does it again.
>
> > But that would mean the defaults for the guest would have to be
> > different than for the host. E.g.,
> > host: ",ipv4" means both,
>
> Why would it mean both? I don't follow you here.
>
> > whereas guest: ",ipv4" (ideally) means ipv4 (since both is meaningless)
>


I guess one has to define:
- how these flags work,
- do they have defaults,
- and if they do have defaults what is the default value?

For the host, neither flag present means "both are on", which could mean,
effectively, that the defaults for
ipv4[={off,on}] and ipv6[={off,on}] are both "on".
[Assuming they have defaults. See above: Do they?
For the guest ipv4=on,ipv6=on is an error.]
But does that then mean that the presence of only "ipv4=on" or "ipv6=on" is
a no-op?
After all, why specify "ipv4=on" at all if it's the default?
I think a reader would get confused if they saw only one of "ipv4=on" or
"ipv6=on"
specified and learned that both were on.
It also means that to specify only one of ipv4 or ipv6 you have to turn the
other off.
It's a bit awkward, but it is consistent and easy to explain (if awkward to
use and read).

On the other hand, for the host, one could, for example,
make these flags tri-state (or call it whatever).
Is specifying only "ipv4=off" the equivalent of specifying only "ipv6=on"?
Presumably it must (it makes the most sense).
There is also the invalid setting of ipv4=off,ipv6=off.
One also needs to specify the order in which the flags are processed,
to define what ipv6=on,ipv6=off means.
Either that or document that specifying them multiple times is undefined.

This is getting a bit verbose to have to explain in documentation,
but it is what it is.
I don't have a say in the decision. I just need to know what to implement.

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

[PATCH v4 3/4] net/slirp.c: Refactor address parsing

[Doug Evans via]

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

New test: avocado run tests/acceptance/hostfwd.py

Signed-off-by: Doug Evans <dje@google.com>
---

Changes from v3:
- this patch renamed from 2/3 to 3/4
- call inet_parse_host_and_port from util/qemu-sockets.c
- added tests/acceptance/hostfwd.py

Changes from v2:
- nothing of consequence

Changes from v1:
- this patch is new in v2
  - address parsing refactor split out, ipv4 changes here
- libslirp part is now upstream in libslirp repo

 net/slirp.c                 | 164 ++++++++++++++++++++++--------------
 tests/acceptance/hostfwd.py |  84 ++++++++++++++++++
 2 files changed, 185 insertions(+), 63 deletions(-)
 create mode 100644 tests/acceptance/hostfwd.py

diff --git a/net/slirp.c b/net/slirp.c
index be914c0be0..ee39014a71 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -631,15 +631,90 @@ static SlirpState *slirp_lookup(Monitor *mon, const char *id)
     }
 }
 
+/*
+ * 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 in *errp.
+ */
+static const char *parse_protocol(const char *str, int sep, bool *is_udp,
+                                  Error **errp)
+{
+    char buf[10];
+    const char *p = str;
+
+    if (get_str_sep(buf, sizeof(buf), &p, sep) < 0) {
+        error_setg(errp, "Missing protocol name separator");
+        return NULL;
+    }
+
+    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
+        *is_udp = false;
+    } else if (!strcmp(buf, "udp")) {
+        *is_udp = true;
+    } else {
+        error_setg(errp, "Bad protocol name");
+        return NULL;
+    }
+
+    return p;
+}
+
+/*
+ * Parse an ip address/port of the form "address:port<terminator>".
+ * 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 in *errp.
+ */
+static const char *parse_ip_addr_and_port(const char *str, int terminator,
+                                          struct in_addr *addr, int *port,
+                                          Error **errp)
+{
+    g_autofree char *addr_str = NULL;
+    g_autofree char *port_str = NULL;
+    bool is_v6;
+    const char *p = inet_parse_host_and_port(str, terminator, &addr_str,
+                                             &port_str, &is_v6, errp);
+
+    if (p == NULL) {
+        return NULL;
+    }
+
+    /* Ignore is_v6 for the moment, if inet_aton fails let it. */
+    if (addr_str[0] == '\0') {
+        addr->s_addr = INADDR_ANY;
+    } else if (!inet_aton(addr_str, addr)) {
+        error_setg(errp, "Bad address");
+        return NULL;
+    }
+
+    if (qemu_strtoi(port_str, NULL, 10, port) < 0 ||
+        *port < 0 || *port > 65535) {
+        error_setg(errp, "Bad port");
+        return NULL;
+    }
+
+    /*
+     * At this point "p" points to the terminator or trailing NUL if the
+     * terminator is not present.
+     */
+    if (*p) {
+        ++p;
+    }
+    return p;
+}
+
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 {
-    struct in_addr host_addr = { .s_addr = INADDR_ANY };
+    struct in_addr host_addr;
     int host_port;
-    char buf[256];
     const char *src_str, *p;
     SlirpState *s;
-    int is_udp = 0;
+    bool is_udp;
     int err;
+    Error *error = NULL;
     const char *arg1 = qdict_get_str(qdict, "arg1");
     const char *arg2 = qdict_get_try_str(qdict, "arg2");
 
@@ -654,30 +729,18 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
         return;
     }
 
+    g_assert(src_str != NULL);
     p = src_str;
-    if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        goto fail_syntax;
-    }
-
-    if (!strcmp(buf, "tcp") || buf[0] == '\0') {
-        is_udp = 0;
-    } else if (!strcmp(buf, "udp")) {
-        is_udp = 1;
-    } else {
-        goto fail_syntax;
-    }
 
-    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, &error);
+    if (p == NULL) {
         goto fail_syntax;
     }
 
-    if (qemu_strtoi(p, NULL, 10, &host_port)) {
+    if (parse_ip_addr_and_port(p, '\0', &host_addr, &host_port,
+                               &error) == NULL) {
         goto fail_syntax;
     }
-
     err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
 
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
@@ -685,65 +748,39 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
     return;
 
  fail_syntax:
-    monitor_printf(mon, "invalid format\n");
+    monitor_printf(mon, "Invalid format: %s\n", error_get_pretty(error));
+    error_free(error);
 }
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
-    struct in_addr host_addr = { .s_addr = INADDR_ANY };
-    struct in_addr guest_addr = { .s_addr = 0 };
+    struct in_addr host_addr, guest_addr;
     int host_port, guest_port;
     const char *p;
-    char buf[256];
-    int is_udp;
-    char *end;
-    const char *fail_reason = "Unknown reason";
+    bool is_udp;
+    Error *error = 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, &error);
+    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_ip_addr_and_port(p, '-', &host_addr, &host_port, &error);
+    if (p == NULL) {
+        error_prepend(&error, "For host address: ");
         goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-        fail_reason = "Missing guest address";
+    if (parse_ip_addr_and_port(p, '\0', &guest_addr, &guest_port,
+                               &error) == NULL) {
+        error_prepend(&error, "For guest address: ");
         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) {
+        error_setg(&error, "For guest address: Bad port");
         goto fail_syntax;
     }
 
@@ -757,7 +794,8 @@ 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);
+               error_get_pretty(error));
+    error_free(error);
     return -1;
 }
 
diff --git a/tests/acceptance/hostfwd.py b/tests/acceptance/hostfwd.py
new file mode 100644
index 0000000000..1eaa0371ba
--- /dev/null
+++ b/tests/acceptance/hostfwd.py
@@ -0,0 +1,84 @@
+# Hostfwd command tests
+#
+# Copyright 2021 Google LLC
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by the
+# Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+# for more details.
+
+
+from avocado_qemu import Test
+
+
+class Hostfwd(Test):
+    """
+    :avocado: tags=hostfwd
+    """
+    def hmc(self, cmd):
+        return self.vm.command('human-monitor-command', command_line=cmd)
+
+    def test_qmp_hostfwd_ipv4(self):
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_add vnet tcp::65022-:22'), '')
+        self.assertEquals(self.hmc('hostfwd_remove vnet tcp::65022'),
+                          'host forwarding rule for tcp::65022 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_add tcp::65022-:22'), '')
+        self.assertEquals(self.hmc('hostfwd_remove tcp::65022'),
+                          'host forwarding rule for tcp::65022 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_add udp::65042-:42'), '')
+        self.assertEquals(self.hmc('hostfwd_remove udp::65042'),
+                          'host forwarding rule for udp::65042 removed\r\n')
+
+    def test_qmp_hostfwd_ipv4_functional_errors(self):
+        """Verify handling of various kinds of errors given valid addresses."""
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_remove ::65022'),
+                          'host forwarding rule for ::65022 not found\r\n')
+        self.assertEquals(self.hmc('hostfwd_add udp::65042-:42'), '')
+        self.assertEquals(self.hmc('hostfwd_add udp::65042-:42'),
+                          "Could not set up host forwarding rule 'udp::65042-:42'\r\n")
+        self.assertEquals(self.hmc('hostfwd_remove ::65042'),
+                          'host forwarding rule for ::65042 not found\r\n')
+        self.assertEquals(self.hmc('hostfwd_remove udp::65042'),
+                          'host forwarding rule for udp::65042 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_remove udp::65042'),
+                          'host forwarding rule for udp::65042 not found\r\n')
+
+    def test_qmp_hostfwd_ipv4_parsing_errors(self):
+        """Verify handling of various kinds of parsing errors."""
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_remove abc::42'),
+                          'Invalid format: Bad protocol name\r\n')
+        self.assertEquals(self.hmc('hostfwd_add abc::65022-:22'),
+                          "Invalid host forwarding rule 'abc::65022-:22' (Bad protocol name)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :a.b.c.d:66-:66'),
+                          "Invalid host forwarding rule ':a.b.c.d:66-:66' (For host address: Bad address)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66-a.b.c.d:66'),
+                          "Invalid host forwarding rule '::66-a.b.c.d:66' (For guest address: Bad address)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66666-:66666'),
+                          "Invalid host forwarding rule '::66666-:66666' (For host address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::-1-foo'),
+                          "Invalid host forwarding rule '::-1-foo' (For host address: error parsing host:port ':')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66-foo'),
+                          "Invalid host forwarding rule '::66-foo' (For guest address: error parsing host:port 'foo')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66-:66666'),
+                          "Invalid host forwarding rule '::66-:66666' (For guest address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66-:-1'),
+                          "Invalid host forwarding rule '::66-:-1' (For guest address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add ::66-:0'),
+                          "Invalid host forwarding rule '::66-:0' (For guest address: Bad port)\r\n")
-- 
2.30.0.617.g56c4b15f3c-goog

[PATCH v4 4/4] net: Extend host forwarding to support IPv6

[Doug Evans via]

Net option "-hostfwd" now supports IPv6 addresses.
Commands hostfwd_add, hostfwd_remove now support IPv6 addresses.

Signed-off-by: Doug Evans <dje@google.com>
---

Differences from v3:
- this patch renamed from 3/3 to 4/4
- ipv6 support added to existing hostfwd option, commands
  - instead of creating new ipv6 option, commands
- added tests to tests/acceptance/hostfwd.py

Differences from v2:
- clarify spelling of ipv6 addresses in docs
- tighten parsing of ipv6 addresses

Differences from v1:
- parsing refactor split out into separate patch (2/3)

 hmp-commands.hx             | 15 +++++++
 net/slirp.c                 | 78 ++++++++++++++++++++++++++-----------
 tests/acceptance/hostfwd.py | 66 +++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+), 23 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..4de4e4979d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1375,6 +1375,16 @@ ERST
 SRST
 ``hostfwd_add``
   Redirect TCP or UDP connections from host to guest (requires -net user).
+  IPV6 addresses are wrapped in square brackes, IPV4 addresses are not.
+
+  Examples:
+  hostfwd_add net0 tcp:127.0.0.1:10022-:22
+  hostfwd_add net0 tcp:[::1]:10022-[fe80::1:2:3:4]:22
+
+  Note that Libslirp currently only provides a "stateless" DHCPv6 server, a
+  consequence of which is that it cannot do the "addr-any" translation to the
+  guest address that is done for IPv4. In other words, the following is
+  currently not supported: hostfwd_add net0 tcp:[::1]:10022-:22
 ERST
 
 #ifdef CONFIG_SLIRP
@@ -1390,6 +1400,11 @@ ERST
 SRST
 ``hostfwd_remove``
   Remove host-to-guest TCP or UDP redirection.
+  IPV6 addresses are wrapped in square brackes, IPV4 addresses are not.
+
+  Examples:
+  hostfwd_remove net0 tcp:127.0.0.1:10022
+  hostfwd_remove net0 tcp:[::1]:10022
 ERST
 
     {
diff --git a/net/slirp.c b/net/slirp.c
index ee39014a71..d8c675a2e8 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -96,6 +96,11 @@ typedef struct SlirpState {
     GSList *fwd;
 } SlirpState;
 
+union in4or6_addr {
+    struct in_addr addr4;
+    struct in6_addr addr6;
+};
+
 static struct slirp_config_str *slirp_configs;
 static QTAILQ_HEAD(, SlirpState) slirp_stacks =
     QTAILQ_HEAD_INITIALIZER(slirp_stacks);
@@ -663,31 +668,38 @@ static const char *parse_protocol(const char *str, int sep, bool *is_udp,
 
 /*
  * Parse an ip address/port of the form "address:port<terminator>".
- * 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.
+ * An empty address means INADDR_ANY/in6addr_any.
+ * Returns a pointer to after the terminator, unless it was '\0', and stores
+ * the results in *addr, *port, *is_v6.
  * Otherwise returns NULL and stores the error in *errp.
  */
 static const char *parse_ip_addr_and_port(const char *str, int terminator,
-                                          struct in_addr *addr, int *port,
-                                          Error **errp)
+                                          union in4or6_addr *addr, int *port,
+                                          bool *is_v6, Error **errp)
 {
     g_autofree char *addr_str = NULL;
     g_autofree char *port_str = NULL;
-    bool is_v6;
     const char *p = inet_parse_host_and_port(str, terminator, &addr_str,
-                                             &port_str, &is_v6, errp);
+                                             &port_str, is_v6, errp);
 
     if (p == NULL) {
         return NULL;
     }
 
-    /* Ignore is_v6 for the moment, if inet_aton fails let it. */
-    if (addr_str[0] == '\0') {
-        addr->s_addr = INADDR_ANY;
-    } else if (!inet_aton(addr_str, addr)) {
-        error_setg(errp, "Bad address");
-        return NULL;
+    if (*is_v6) {
+        if (addr_str[0] == '\0') {
+            addr->addr6 = in6addr_any;
+        } else if (!inet_pton(AF_INET6, addr_str, &addr->addr6)) {
+            error_setg(errp, "Bad address");
+            return NULL;
+        }
+    } else {
+        if (addr_str[0] == '\0') {
+            addr->addr4.s_addr = INADDR_ANY;
+        } else if (!inet_pton(AF_INET, addr_str, &addr->addr4)) {
+            error_setg(errp, "Bad address");
+            return NULL;
+        }
     }
 
     if (qemu_strtoi(port_str, NULL, 10, port) < 0 ||
@@ -708,11 +720,11 @@ static const char *parse_ip_addr_and_port(const char *str, int terminator,
 
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 {
-    struct in_addr host_addr;
+    union in4or6_addr host_addr;
     int host_port;
     const char *src_str, *p;
     SlirpState *s;
-    bool is_udp;
+    bool is_udp, is_v6;
     int err;
     Error *error = NULL;
     const char *arg1 = qdict_get_str(qdict, "arg1");
@@ -737,11 +749,18 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
         goto fail_syntax;
     }
 
-    if (parse_ip_addr_and_port(p, '\0', &host_addr, &host_port,
+    if (parse_ip_addr_and_port(p, '\0', &host_addr, &host_port, &is_v6,
                                &error) == NULL) {
         goto fail_syntax;
     }
-    err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
+
+    if (is_v6) {
+        err = slirp_remove_ipv6_hostfwd(s->slirp, is_udp, host_addr.addr6,
+                                        host_port);
+    } else {
+        err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr.addr4,
+                                   host_port);
+    }
 
     monitor_printf(mon, "host forwarding rule for %s %s\n", src_str,
                    err ? "not found" : "removed");
@@ -754,11 +773,12 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
 
 static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
-    struct in_addr host_addr, guest_addr;
+    union in4or6_addr host_addr, guest_addr;
     int host_port, guest_port;
     const char *p;
-    bool is_udp;
+    bool is_udp, host_is_v6, guest_is_v6;
     Error *error = NULL;
+    int err;
 
     g_assert(redir_str != NULL);
     p = redir_str;
@@ -768,24 +788,36 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
         goto fail_syntax;
     }
 
-    p = parse_ip_addr_and_port(p, '-', &host_addr, &host_port, &error);
+    p = parse_ip_addr_and_port(p, '-', &host_addr, &host_port, &host_is_v6,
+                               &error);
     if (p == NULL) {
         error_prepend(&error, "For host address: ");
         goto fail_syntax;
     }
 
-    if (parse_ip_addr_and_port(p, '\0', &guest_addr, &guest_port,
+    if (parse_ip_addr_and_port(p, '\0', &guest_addr, &guest_port, &guest_is_v6,
                                &error) == NULL) {
         error_prepend(&error, "For guest address: ");
         goto fail_syntax;
     }
+    if (host_is_v6 != guest_is_v6) {
+        /* TODO: Can libslirp support this? */
+        error_setg(&error, "Both host,guest must be one of ipv4 or ipv6");
+        goto fail_syntax;
+    }
     if (guest_port == 0) {
         error_setg(&error, "For guest address: Bad port");
         goto fail_syntax;
     }
 
-    if (slirp_add_hostfwd(s->slirp, is_udp, host_addr, host_port, guest_addr,
-                          guest_port) < 0) {
+    if (host_is_v6) {
+        err = slirp_add_ipv6_hostfwd(s->slirp, is_udp, host_addr.addr6,
+                                     host_port, guest_addr.addr6, guest_port);
+    } else {
+        err = slirp_add_hostfwd(s->slirp, is_udp, host_addr.addr4, host_port,
+                                guest_addr.addr4, guest_port);
+    }
+    if (err < 0) {
         error_setg(errp, "Could not set up host forwarding rule '%s'",
                    redir_str);
         return -1;
diff --git a/tests/acceptance/hostfwd.py b/tests/acceptance/hostfwd.py
index 1eaa0371ba..4368d557c9 100644
--- a/tests/acceptance/hostfwd.py
+++ b/tests/acceptance/hostfwd.py
@@ -82,3 +82,69 @@ def test_qmp_hostfwd_ipv4_parsing_errors(self):
                           "Invalid host forwarding rule '::66-:-1' (For guest address: Bad port)\r\n")
         self.assertEquals(self.hmc('hostfwd_add ::66-:0'),
                           "Invalid host forwarding rule '::66-:0' (For guest address: Bad port)\r\n")
+
+    def test_qmp_hostfwd_ipv6(self):
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_add vnet tcp:[::1]:65022-[fe80::1]:22'),
+                          '')
+        self.assertEquals(self.hmc('hostfwd_remove vnet tcp:[::1]:65022'),
+                          'host forwarding rule for tcp:[::1]:65022 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_add tcp:[]:65042-[fe80::1]:42'),
+                          '')
+        self.assertEquals(self.hmc('hostfwd_remove tcp:[]:65042'),
+                          'host forwarding rule for tcp:[]:65042 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_add udp:[::1]:65042-[fe80::1]:42'),
+                          '')
+        self.assertEquals(self.hmc('hostfwd_remove udp:[::1]:65042'),
+                          'host forwarding rule for udp:[::1]:65042 removed\r\n')
+
+    def test_qmp_hostfwd_ipv6_functional_errors(self):
+        """Verify handling of various kinds of errors given valid addresses."""
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_remove :[::1]:65022'),
+                          'host forwarding rule for :[::1]:65022 not found\r\n')
+        self.assertEquals(self.hmc('hostfwd_add udp:[::1]:65042-[fe80::1]:42'),
+                          '')
+        self.assertEquals(self.hmc('hostfwd_add udp:[::1]:65042-[fe80::1]:42'),
+                          "Could not set up host forwarding rule 'udp:[::1]:65042-[fe80::1]:42'\r\n")
+        self.assertEquals(self.hmc('hostfwd_remove :[::1]:65042'),
+                          'host forwarding rule for :[::1]:65042 not found\r\n')
+        self.assertEquals(self.hmc('hostfwd_remove udp:[::1]:65042'),
+                          'host forwarding rule for udp:[::1]:65042 removed\r\n')
+        self.assertEquals(self.hmc('hostfwd_remove udp:[::1]:65042'),
+                          'host forwarding rule for udp:[::1]:65042 not found\r\n')
+
+    def test_qmp_hostfwd_ipv6_errors(self):
+        """Verify handling of various kinds of errors."""
+        self.vm.add_args('-nodefaults',
+                         '-netdev', 'user,id=vnet',
+                         '-device', 'virtio-net,netdev=vnet')
+        self.vm.launch()
+        self.assertEquals(self.hmc('hostfwd_add :[::1-'),
+                          "Invalid host forwarding rule ':[::1-' (For host address: error parsing IPv6 host:port '[::1')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-[fe80::1'),
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1' (For guest address: error parsing IPv6 host:port '[fe80::1')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[:::]:66-foo'),
+                          "Invalid host forwarding rule ':[:::]:66-foo' (For host address: Bad address)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]-foo'),
+                          "Invalid host forwarding rule ':[::1]-foo' (For host address: error parsing IPv6 host:port '[::1]')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-[foo]'),
+                          "Invalid host forwarding rule ':[::1]:66-[foo]' (For guest address: error parsing IPv6 host:port '[foo]')\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66666-foo'),
+                          "Invalid host forwarding rule ':[::1]:66666-foo' (For host address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-[fe80::1]:-1'),
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:-1' (For guest address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-[fe80::1]:66666'),
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:66666' (For guest address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-[fe80::1]:0'),
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:0' (For guest address: Bad port)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :[::1]:66-1.2.3.4:66'),
+                          "Invalid host forwarding rule ':[::1]:66-1.2.3.4:66' (Both host,guest must be one of ipv4 or ipv6)\r\n")
+        self.assertEquals(self.hmc('hostfwd_add :1.2.3.4:66-[fe80::1]:66'),
+                          "Invalid host forwarding rule ':1.2.3.4:66-[fe80::1]:66' (Both host,guest must be one of ipv4 or ipv6)\r\n")
-- 
2.30.0.617.g56c4b15f3c-goog

Re: [PATCH v4 0/4] Add support for ipv6 host forwarding

[no-reply]

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



Hi,

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

Type: series
Message-id: 20210218201538.701509-1-dje@google.com
Subject: [PATCH v4 0/4] 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/20210218201538.701509-1-dje@google.com -> patchew/20210218201538.701509-1-dje@google.com
Switched to a new branch 'test'
2998bb9b net: Extend host forwarding to support IPv6
2b46856 net/slirp.c: Refactor address parsing
61d8583 util/qemu-sockets.c: Split host:port parsing out of inet_parse
ad280b5 slirp: Advance libslirp submodule to add ipv6 host-forward support

=== OUTPUT BEGIN ===
1/4 Checking commit ad280b59a66e (slirp: Advance libslirp submodule to add ipv6 host-forward support)
2/4 Checking commit 61d85833d837 (util/qemu-sockets.c: Split host:port parsing out of inet_parse)
ERROR: "foo* bar" should be "foo *bar"
#25: FILE: include/qemu/sockets.h:34:
+const char* inet_parse_host_and_port(const char* str, int terminator,

ERROR: "foo* bar" should be "foo *bar"
#50: FILE: util/qemu-sockets.c:629:
+const char* inet_parse_host_and_port(const char* str, int terminator,

total: 2 errors, 0 warnings, 123 lines checked

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

3/4 Checking commit 2b46856db1e0 (net/slirp.c: Refactor address parsing)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#247: 
new file mode 100644

WARNING: line over 80 characters
#302: FILE: tests/acceptance/hostfwd.py:51:
+                          "Could not set up host forwarding rule 'udp::65042-:42'\r\n")

ERROR: line over 90 characters
#319: FILE: tests/acceptance/hostfwd.py:68:
+                          "Invalid host forwarding rule 'abc::65022-:22' (Bad protocol name)\r\n")

ERROR: line over 90 characters
#321: FILE: tests/acceptance/hostfwd.py:70:
+                          "Invalid host forwarding rule ':a.b.c.d:66-:66' (For host address: Bad address)\r\n")

ERROR: line over 90 characters
#323: FILE: tests/acceptance/hostfwd.py:72:
+                          "Invalid host forwarding rule '::66-a.b.c.d:66' (For guest address: Bad address)\r\n")

ERROR: line over 90 characters
#325: FILE: tests/acceptance/hostfwd.py:74:
+                          "Invalid host forwarding rule '::66666-:66666' (For host address: Bad port)\r\n")

ERROR: line over 90 characters
#327: FILE: tests/acceptance/hostfwd.py:76:
+                          "Invalid host forwarding rule '::-1-foo' (For host address: error parsing host:port ':')\r\n")

ERROR: line over 90 characters
#329: FILE: tests/acceptance/hostfwd.py:78:
+                          "Invalid host forwarding rule '::66-foo' (For guest address: error parsing host:port 'foo')\r\n")

ERROR: line over 90 characters
#331: FILE: tests/acceptance/hostfwd.py:80:
+                          "Invalid host forwarding rule '::66-:66666' (For guest address: Bad port)\r\n")

ERROR: line over 90 characters
#333: FILE: tests/acceptance/hostfwd.py:82:
+                          "Invalid host forwarding rule '::66-:-1' (For guest address: Bad port)\r\n")

ERROR: line over 90 characters
#335: FILE: tests/acceptance/hostfwd.py:84:
+                          "Invalid host forwarding rule '::66-:0' (For guest address: Bad port)\r\n")

total: 9 errors, 2 warnings, 304 lines checked

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

4/4 Checking commit 2998bb9bc67e (net: Extend host forwarding to support IPv6)
WARNING: line over 80 characters
#224: FILE: tests/acceptance/hostfwd.py:91:
+        self.assertEquals(self.hmc('hostfwd_add vnet tcp:[::1]:65022-[fe80::1]:22'),

WARNING: line over 80 characters
#227: FILE: tests/acceptance/hostfwd.py:94:
+                          'host forwarding rule for tcp:[::1]:65022 removed\r\n')

WARNING: line over 80 characters
#235: FILE: tests/acceptance/hostfwd.py:102:
+                          'host forwarding rule for udp:[::1]:65042 removed\r\n')

ERROR: line over 90 characters
#248: FILE: tests/acceptance/hostfwd.py:115:
+                          "Could not set up host forwarding rule 'udp:[::1]:65042-[fe80::1]:42'\r\n")

WARNING: line over 80 characters
#252: FILE: tests/acceptance/hostfwd.py:119:
+                          'host forwarding rule for udp:[::1]:65042 removed\r\n')

WARNING: line over 80 characters
#254: FILE: tests/acceptance/hostfwd.py:121:
+                          'host forwarding rule for udp:[::1]:65042 not found\r\n')

ERROR: line over 90 characters
#263: FILE: tests/acceptance/hostfwd.py:130:
+                          "Invalid host forwarding rule ':[::1-' (For host address: error parsing IPv6 host:port '[::1')\r\n")

ERROR: line over 90 characters
#265: FILE: tests/acceptance/hostfwd.py:132:
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1' (For guest address: error parsing IPv6 host:port '[fe80::1')\r\n")

ERROR: line over 90 characters
#267: FILE: tests/acceptance/hostfwd.py:134:
+                          "Invalid host forwarding rule ':[:::]:66-foo' (For host address: Bad address)\r\n")

ERROR: line over 90 characters
#269: FILE: tests/acceptance/hostfwd.py:136:
+                          "Invalid host forwarding rule ':[::1]-foo' (For host address: error parsing IPv6 host:port '[::1]')\r\n")

ERROR: line over 90 characters
#271: FILE: tests/acceptance/hostfwd.py:138:
+                          "Invalid host forwarding rule ':[::1]:66-[foo]' (For guest address: error parsing IPv6 host:port '[foo]')\r\n")

ERROR: line over 90 characters
#273: FILE: tests/acceptance/hostfwd.py:140:
+                          "Invalid host forwarding rule ':[::1]:66666-foo' (For host address: Bad port)\r\n")

ERROR: line over 90 characters
#275: FILE: tests/acceptance/hostfwd.py:142:
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:-1' (For guest address: Bad port)\r\n")

ERROR: line over 90 characters
#277: FILE: tests/acceptance/hostfwd.py:144:
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:66666' (For guest address: Bad port)\r\n")

ERROR: line over 90 characters
#279: FILE: tests/acceptance/hostfwd.py:146:
+                          "Invalid host forwarding rule ':[::1]:66-[fe80::1]:0' (For guest address: Bad port)\r\n")

ERROR: line over 90 characters
#281: FILE: tests/acceptance/hostfwd.py:148:
+                          "Invalid host forwarding rule ':[::1]:66-1.2.3.4:66' (Both host,guest must be one of ipv4 or ipv6)\r\n")

ERROR: line over 90 characters
#283: FILE: tests/acceptance/hostfwd.py:150:
+                          "Invalid host forwarding rule ':1.2.3.4:66-[fe80::1]:66' (Both host,guest must be one of ipv4 or ipv6)\r\n")

total: 12 errors, 5 warnings, 245 lines checked

Patch 4/4 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/20210218201538.701509-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