From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Doug Evans <dje@google.com>
Cc: "Samuel Thibault" <samuel.thibault@ens-lyon.org>,
"Daniel P . Berrangé" <berrange@redhat.com>,
QEMU <qemu-devel@nongnu.org>
Subject: Re: [PATCH v6 3/4] net/slirp.c: Refactor address parsing
Date: Fri, 7 May 2021 19:29:05 +0400 [thread overview]
Message-ID: <CAJ+F1CKwNQ+YCMr+kmquxB32fMkWyXZd+E8deRR96C8V4qte7g@mail.gmail.com> (raw)
In-Reply-To: <20210415033925.1290401-4-dje@google.com>
[-- Attachment #1: Type: text/plain, Size: 17735 bytes --]
Hi
On Thu, Apr 15, 2021 at 7:44 AM Doug Evans <dje@google.com> wrote:
> ... in preparation for adding ipv6 host forwarding support.
>
> Tested:
> avocado run tests/acceptance/hostfwd.py
>
> Signed-off-by: Doug Evans <dje@google.com>
> ---
>
> Changes from v5:
>
> Use InetSocketAddress and getaddrinfo().
> Use new libslirp calls: slirp_remove_hostxfwd, slirp_add_hostxfwd.
>
> include/qemu/sockets.h | 2 +
> net/slirp.c | 200 ++++++++++++++++++++++++------------
> tests/acceptance/hostfwd.py | 91 ++++++++++++++++
> util/qemu-sockets.c | 17 +--
> 4 files changed, 241 insertions(+), 69 deletions(-)
> create mode 100644 tests/acceptance/hostfwd.py
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 94f4e8de83..6fd71775ce 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -29,6 +29,8 @@ int socket_set_fast_reuse(int fd);
> #define SHUT_RDWR 2
> #endif
>
> +int sockaddr_getport(const struct sockaddr *addr);
> +
> int inet_ai_family_from_address(InetSocketAddress *addr,
> Error **errp);
> const char *inet_parse_host_port(InetSocketAddress *addr,
> diff --git a/net/slirp.c b/net/slirp.c
> index a01a0fccd3..4be065c30b 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -641,15 +641,108 @@ static SlirpState *slirp_lookup(Monitor *mon, const
> char *id)
> }
> }
>
> +static const char *parse_protocol(const char *str, bool *is_udp,
> + Error **errp)
> +{
> + char buf[10];
> + const char *p = str;
> +
> + if (get_str_sep(buf, sizeof(buf), &p, ':') < 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 '%s'", buf);
> + return NULL;
> + }
> +
> + return p;
> +}
> +
> +static int parse_hostfwd_sockaddr(const char *str, int socktype,
> + struct sockaddr_storage *saddr,
> + Error **errp)
> +{
> + struct addrinfo hints, *res = NULL, *e;
> + InetSocketAddress *addr = g_new(InetSocketAddress, 1);
> + int gai_rc;
> + int rc = -1;
> +
> + const char *optstr = inet_parse_host_port(addr, str, errp);
> + if (optstr == NULL) {
> + goto fail_return;
> + }
> +
> + memset(&hints, 0, sizeof(hints));
> + hints.ai_flags = AI_PASSIVE; /* ignored if host is not ""(->NULL) */
> + hints.ai_flags |= AI_NUMERICHOST | AI_NUMERICSERV;
> + hints.ai_socktype = socktype;
> + hints.ai_family = PF_INET;
> +
> + /*
> + * Calling getaddrinfo for guest addresses is dubious, but addresses
> are
> + * restricted to numeric only. Convert "" to NULL for getaddrinfo's
> + * benefit.
> + */
> + gai_rc = getaddrinfo(*addr->host ? addr->host : NULL,
> + *addr->port ? addr->port : NULL, &hints, &res);
> + if (gai_rc != 0) {
> + error_setg(errp, "address resolution failed for '%s': %s",
> + str, gai_strerror(gai_rc));
> + goto fail_return;
> + }
> + if (res->ai_next != NULL) {
> + /*
> + * The caller only wants one address, and except for "any" for
> both
> + * ipv4 and ipv6 (which we've already precluded above), we
> shouldn't
> + * get more than one. To assist debugging print all we find.
> + */
> + GString *s = g_string_new(NULL);
> + for (e = res; e != NULL; e = e->ai_next) {
> + char host[NI_MAXHOST];
> + char serv[NI_MAXSERV];
> + int ret = getnameinfo((struct sockaddr *)e->ai_addr,
> e->ai_addrlen,
> + host, sizeof(host),
> + serv, sizeof(serv),
> + NI_NUMERICHOST | NI_NUMERICSERV);
> + if (ret == 0) {
> + g_string_append_printf(s, "\n %s:%s", host, serv);
> + } else {
> + g_string_append_printf(s, "\n unknown, got: %s",
> + gai_strerror(ret));
> + }
> + }
> + error_setg(errp, "multiple addresses resolved for '%s':%s",
> + str, s->str);
> + g_string_free(s, TRUE);
> + goto fail_return;
> + }
> +
> + memcpy(saddr, res->ai_addr, res->ai_addrlen);
> + rc = 0;
> +
> + fail_return:
> + qapi_free_InetSocketAddress(addr);
> + if (res) {
> + freeaddrinfo(res);
> + }
> + return rc;
> +}
> +
> void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
> {
> - struct in_addr host_addr = { .s_addr = INADDR_ANY };
> - int host_port;
> - char buf[256];
> + struct sockaddr_storage host_addr;
> const char *src_str, *p;
> SlirpState *s;
> - int is_udp = 0;
> + bool is_udp;
> + Error *error = NULL;
> int err;
> + int flags = 0;
> const char *arg1 = qdict_get_str(qdict, "arg1");
> const char *arg2 = qdict_get_try_str(qdict, "arg2");
>
> @@ -664,110 +757,91 @@ 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 {
> + p = parse_protocol(p, &is_udp, &error);
> + if (p == NULL) {
> goto fail_syntax;
> }
> -
> - if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
> - goto fail_syntax;
> - }
> - if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
> - goto fail_syntax;
> + if (is_udp) {
> + flags |= SLIRP_HOSTFWD_UDP;
>
This fails to build with the system version of libslirp, as there has not
been releases with this new symbol yet. I am not sure what's the status for
upstream release, we should create an issue there and discuss it.
Anyway, you'll probably need to make the new code conditional with
SLIRP_CHECK_VERSION(), as we don't want qemu to depend on too recent
releases.
thanks
}
>
> - if (qemu_strtoi(p, NULL, 10, &host_port)) {
> + if (parse_hostfwd_sockaddr(p, is_udp ? SOCK_DGRAM : SOCK_STREAM,
> + &host_addr, &error) < 0) {
> goto fail_syntax;
> }
>
> - err = slirp_remove_hostfwd(s->slirp, is_udp, host_addr, host_port);
> + err = slirp_remove_hostxfwd(s->slirp, (struct sockaddr *) &host_addr,
> + sizeof(host_addr), flags);
>
> 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", 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 };
> - int host_port, guest_port;
> + struct sockaddr_storage host_addr, guest_addr;
> const char *p;
> char buf[256];
> - int is_udp;
> - char *end;
> - const char *fail_reason = "Unknown reason";
> + bool is_udp;
> + Error *error = NULL;
> + int flags = 0;
> + int port;
>
> + 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";
> + p = parse_protocol(p, &is_udp, &error);
> + if (p == NULL) {
> goto fail_syntax;
> }
> - if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
> - fail_reason = "Bad host address";
> - goto fail_syntax;
> + if (is_udp) {
> + flags |= SLIRP_HOSTFWD_UDP;
> }
>
> 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";
> + error_setg(&error, "missing host-guest separator");
> goto fail_syntax;
> }
>
> - if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
> - fail_reason = "Missing guest address";
> + if (parse_hostfwd_sockaddr(buf, is_udp ? SOCK_DGRAM : SOCK_STREAM,
> + &host_addr, &error) < 0) {
> + error_prepend(&error, "For host address: ");
> goto fail_syntax;
> }
> - if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
> - fail_reason = "Bad guest address";
> +
> + if (parse_hostfwd_sockaddr(p, is_udp ? SOCK_DGRAM : SOCK_STREAM,
> + &guest_addr, &error) < 0) {
> + error_prepend(&error, "For 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";
> + port = sockaddr_getport((struct sockaddr *) &guest_addr);
> + if (port == 0) {
> + error_setg(&error, "For guest address: invalid port '0'");
> goto fail_syntax;
> }
>
> - if (slirp_add_hostfwd(s->slirp, is_udp, host_addr, host_port,
> guest_addr,
> - guest_port) < 0) {
> - error_setg(errp, "Could not set up host forwarding rule '%s'",
> - redir_str);
> + if (slirp_add_hostxfwd(s->slirp,
> + (struct sockaddr *) &host_addr,
> sizeof(host_addr),
> + (struct sockaddr *) &guest_addr,
> sizeof(guest_addr),
> + flags) < 0) {
> + error_setg(errp, "Could not set up host forwarding rule '%s': %s",
> + redir_str, strerror(errno));
> return -1;
> }
> return 0;
>
> 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..9b9db142c3
> --- /dev/null
> +++ b/tests/acceptance/hostfwd.py
> @@ -0,0 +1,91 @@
> +# 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': Address already in use\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 'abc'\r\n")
> + self.assertEquals(self.hmc('hostfwd_add abc::65022-:22'),
> + "Invalid host forwarding rule 'abc::65022-:22'"
> + \
> + " (bad protocol name 'abc')\r\n")
> + self.assertEquals(self.hmc('hostfwd_add :foo'),
> + "Invalid host forwarding rule ':foo'" + \
> + " (missing host-guest separator)\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: address resolution failed
> for" \
> + " 'a.b.c.d:66': Name or service not known)\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: address resolution
> failed" + \
> + " for 'a.b.c.d:66': Name or service not
> known)\r\n")
> + self.assertEquals(self.hmc('hostfwd_add ::-1-foo'),
> + "Invalid host forwarding rule '::-1-foo'" + \
> + " (For host address: error parsing port in" + \
> + " address ':')\r\n")
> + self.assertEquals(self.hmc('hostfwd_add ::66-foo'),
> + "Invalid host forwarding rule '::66-foo' (For"
> + \
> + " guest address: error parsing address
> 'foo')\r\n")
> + self.assertEquals(self.hmc('hostfwd_add ::66-:0'),
> + "Invalid host forwarding rule '::66-:0'" + \
> + " (For guest address: invalid port '0')\r\n")
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index c0069f2565..983efeee2f 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -46,23 +46,28 @@
> #endif
>
>
> -static int inet_getport(struct addrinfo *e)
> +int sockaddr_getport(const struct sockaddr *addr)
> {
> - struct sockaddr_in *i4;
> - struct sockaddr_in6 *i6;
> + const struct sockaddr_in *i4;
> + const struct sockaddr_in6 *i6;
>
> - switch (e->ai_family) {
> + switch (addr->sa_family) {
> case PF_INET6:
> - i6 = (void*)e->ai_addr;
> + i6 = (void *)addr;
> return ntohs(i6->sin6_port);
> case PF_INET:
> - i4 = (void*)e->ai_addr;
> + i4 = (void *)addr;
> return ntohs(i4->sin_port);
> default:
> return 0;
> }
> }
>
> +static int inet_getport(struct addrinfo *e)
> +{
> + return sockaddr_getport(e->ai_addr);
> +}
> +
> static void inet_setport(struct addrinfo *e, int port)
> {
> struct sockaddr_in *i4;
> --
> 2.31.1.295.g9ea45b61b8-goog
>
>
>
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 22452 bytes --]
next prev parent reply other threads:[~2021-05-07 15:30 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-15 3:39 [PATCH v6 0/4] Add support for ipv6 host forwarding Doug Evans
2021-04-15 3:39 ` [PATCH v6 1/4] slirp: Advance libslirp submodule to add ipv6 host-forward support Doug Evans
2021-05-07 15:23 ` Marc-André Lureau
2021-05-07 15:46 ` Doug Evans
2021-05-12 16:42 ` Doug Evans
2021-05-12 17:18 ` Marc-André Lureau
2021-05-12 19:50 ` Doug Evans
2021-05-12 20:14 ` Marc-André Lureau
2021-04-15 3:39 ` [PATCH v6 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse Doug Evans
2021-05-07 15:23 ` Marc-André Lureau
2021-05-25 19:37 ` RFC: IPv6 hostfwd command line syntax [was Re: [PATCH v6 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse] Doug Evans
2021-05-26 13:57 ` Daniel P. Berrangé
2021-05-26 15:26 ` Doug Evans
2021-05-26 15:29 ` Daniel P. Berrangé
2021-04-15 3:39 ` [PATCH v6 3/4] net/slirp.c: Refactor address parsing Doug Evans
2021-04-15 15:36 ` Doug Evans
2021-05-07 15:29 ` Marc-André Lureau [this message]
2021-04-15 3:39 ` [PATCH v6 4/4] net: Extend host forwarding to support IPv6 Doug Evans
2021-04-29 3:37 ` [PATCH v6 0/4] Add support for ipv6 host forwarding Doug Evans
2021-05-05 15:21 ` Doug Evans
2021-05-05 16:13 ` Philippe Mathieu-Daudé
2021-05-05 16:15 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJ+F1CKwNQ+YCMr+kmquxB32fMkWyXZd+E8deRR96C8V4qte7g@mail.gmail.com \
--to=marcandre.lureau@gmail.com \
--cc=berrange@redhat.com \
--cc=dje@google.com \
--cc=qemu-devel@nongnu.org \
--cc=samuel.thibault@ens-lyon.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.