diff mbox series

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

Message ID 20210203213729.1940893-2-dje@google.com (mailing list archive)
State New, archived
Headers show
Series Add support for ipv6 host forwarding | expand

Commit Message

Denis V. Lunev" via Feb. 3, 2021, 9:37 p.m. UTC 
... in preparation for adding ipv6 host forwarding support.
---
 net/slirp.c | 200 +++++++++++++++++++++++++++++++++-------------------
 slirp       |   2 +-
 2 files changed, 130 insertions(+), 72 deletions(-)

Comments

Doug Evans Feb. 3, 2021, 9:39 p.m. UTC | #1 
On Wed, Feb 3, 2021 at 1:37 PM Doug Evans <dje@google.com> wrote:

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



Yeah, the Signed-off-by line is missing here. Will add in next version, but
will wait for further comments.
Samuel Thibault Feb. 3, 2021, 10:15 p.m. UTC | #2 
Doug Evans, le mer. 03 févr. 2021 13:37:28 -0800, a ecrit:
> ... in preparation for adding ipv6 host forwarding support.

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

except

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

Which, I would say, deserves its own commit?
Doug Evans Feb. 3, 2021, 10:31 p.m. UTC | #3 
On Wed, Feb 3, 2021 at 2:15 PM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

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


Yep. Still getting used to patch submission in the presence of submodules
...
Philippe Mathieu-Daudé Feb. 8, 2021, 11:09 a.m. UTC | #4 
Hi Doug,

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

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

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

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

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

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

fatal: bad object 358c0827d49778f016312bfb4167fe639900681f
Doug Evans Feb. 8, 2021, 6:59 p.m. UTC | #5 
On Mon, Feb 8, 2021 at 3:09 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

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


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

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


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


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

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

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

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

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

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

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

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

Samuel
diff mbox series

Patch

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