Hi
On Thu, Apr 15, 2021 at 7:44 AM Doug Evans <d...@google.com> wrote:
> ... in preparation for adding ipv6 host forwarding support.
>
> Tested:
> avocado run tests/acceptance/hostfwd.py
>
> Signed-off-by: Doug Evans <d...@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
