From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-20.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F26FEC433E0 for ; Tue, 23 Feb 2021 18:24:46 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3EB9164E57 for ; Tue, 23 Feb 2021 18:24:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3EB9164E57 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:46340 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lEcMW-0007DI-Tr for qemu-devel@archiver.kernel.org; Tue, 23 Feb 2021 13:24:44 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:38322) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lEcLm-0006JM-2Y for qemu-devel@nongnu.org; Tue, 23 Feb 2021 13:23:58 -0500 Received: from mail-ua1-x931.google.com ([2607:f8b0:4864:20::931]:41460) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lEcLj-0006Sb-6R for qemu-devel@nongnu.org; Tue, 23 Feb 2021 13:23:57 -0500 Received: by mail-ua1-x931.google.com with SMTP id w24so657383uau.8 for ; Tue, 23 Feb 2021 10:23:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=8anQUnArPnJOBP7ZRuQhg0gs53K7bKbtdnDs1LSBhOY=; b=tu0So1Uknmcse1wr3QXResS2rq0/2XgVd20UiwRATTcNvvzhIjc7DQcGm+8Fuyvgqt puCs6Ixy4B+1ntyD9mkz2OgkVMRNhb7ys0NVNWdUQHEXHEmdlE5tyVg3yU44fqv7sYNz clzVSl7DONzcK04QDaLPAdEHfk4kdIkuOpfgjR6bU3784CGtjmA6Oy93eF35FuqM5Txa NzfiBwwCW++s+ZH1v0WCwel/aG/UEh/XnkxoHbCHnuCNB46scmQblOHcunGej8Kvg3SB 1f6YizWfXKNyBBuYpTvHnRY9BpYjxKgMr7X6J5nYb2mC1NbciSEDnqXozPCvgH0sZ1iA cyvQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=8anQUnArPnJOBP7ZRuQhg0gs53K7bKbtdnDs1LSBhOY=; b=Q8ROOwGYLtnfYCDlw0ulQvlnpztDnWdGNt0mom09ryezP6wKtNyfGcQoI00na2Popp IlSxYKSj6DE1Qwj6vWllTiZxyLIYjnc1fq6lZjjG7RlS/HV07j32h+aOsZB0B9uaTehL fjapIVdou1Zc1qb9BP4+VHYSsTAodFaryXxbmv36Qnb4m1Hj9DX8TJYwrVMxNps2T5Ed 3c0o+AsAvmzUA4hjU1MvyOVT6DVToc29gFIWGQZBi/KgZ4XttT0fUVL5ZMCCPGVcXNTE 60DZjl/DAH1pJEtXyyMdEihaKgyu1xUFTk95frfewIhOLz01Tsx9Io0cFdayZAbwII6W TQ+g== X-Gm-Message-State: AOAM533RYfDsGnbE5Bg1JLXge6B66E3K1Olfor9FqXLOr1baw3S7qizV w8C97bVR942hAUQq3lWmdom0IIk/T4H6v6HUjl5Arg== X-Google-Smtp-Source: ABdhPJyJUAC1VS3RD0ecmw55viKOgNM/jamiPuGjXZrbfo0X0xDlYgXF9VyXlUA4oU1aIaRJA2ZCMdq0QzoRcmRTozQ= X-Received: by 2002:a9f:2701:: with SMTP id a1mr12424015uaa.120.1614104632957; Tue, 23 Feb 2021 10:23:52 -0800 (PST) MIME-Version: 1.0 References: <20210218201538.701509-1-dje@google.com> <20210218201538.701509-3-dje@google.com> In-Reply-To: From: Doug Evans Date: Tue, 23 Feb 2021 10:23:15 -0800 Message-ID: Subject: Re: [PATCH v4 2/4] util/qemu-sockets.c: Split host:port parsing out of inet_parse To: =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= Cc: QEMU Developers , Samuel Thibault Content-Type: multipart/alternative; boundary="00000000000047a67b05bc050580" Received-SPF: pass client-ip=2607:f8b0:4864:20::931; envelope-from=dje@google.com; helo=mail-ua1-x931.google.com X-Spam_score_int: -175 X-Spam_score: -17.6 X-Spam_bar: ----------------- X-Spam_report: (-17.6 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_MED=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, ENV_AND_HDR_SPF_MATCH=-0.5, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, USER_IN_DEF_DKIM_WL=-7.5, USER_IN_DEF_SPF_WL=-7.5 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000047a67b05bc050580 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Feb 22, 2021 at 1:39 AM Daniel P. Berrang=C3=A9 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=C3=A9 > > 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 > > > > --- > > > > > > > > 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, boo= l > > > *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 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 give= n. > > > 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 =3D strchr(str, terminator); > > > > + g_autofree char *buf =3D NULL; > > > > char host[65]; > > > > char port[33]; > > > > - int to; > > > > - int pos; > > > > - char *begin; > > > > > > > > - memset(addr, 0, sizeof(*addr)); > > > > + if (terminator_ptr =3D=3D NULL) { > > > > + /* If the terminator isn't found then use the entire > string. */ > > > > + terminator_ptr =3D str + strlen(str); > > > > + } > > > > + buf =3D g_strndup(str, terminator_ptr - str); > > > > > > > > - /* parse address */ > > > > - if (str[0] =3D=3D ':') { > > > > - /* no host given */ > > > > - host[0] =3D '\0'; > > > > - if (sscanf(str, ":%32[^,]%n", port, &pos) !=3D 1) { > > > > - error_setg(errp, "error parsing port in address '%s'", > str); > > > > - return -1; > > > > - } > > > > > > > > > > - } else if (str[0] =3D=3D '[') { > > > > + if (buf[0] =3D=3D '[') { > > > > /* IPv6 addr */ > > > > - if (sscanf(str, "[%64[^]]]:%32[^,]%n", host, port, &pos) != =3D > 2) { > > > > - error_setg(errp, "error parsing IPv6 address '%s'", > str); > > > > - return -1; > > > > + if (buf[1] =3D=3D ']') { > > > > + /* sscanf %[ doesn't recognize empty contents. */ > > > > + host[0] =3D '\0'; > > > > + if (sscanf(buf, "[]:%32s", port) !=3D 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[=3Don|off],ipv6[=3Don|off]" later follow. > > > > > > > > > We're creating a new utility subroutine: Let's decide what the API is f= or > > it. > > The fact that inet_parse is passed additional parameters to specify ipv= 4 > 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 h= as > > 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 lat= er > > 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) !=3D 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) != =3D > 2) { > > > > - error_setg(errp, "error parsing address '%s'", str); > > > > - return -1; > > > > + if (buf[0] =3D=3D ':') { > > > > + /* no host given */ > > > > + host[0] =3D '\0'; > > > > + if (sscanf(buf, ":%32s", port) !=3D 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 unsu= re > > > why you moved it ? > > > > > > > + } else { > > > > + /* hostname or IPv4 addr */ > > > > + if (sscanf(buf, "%64[^:]:%32s", host, port) !=3D 2) { > > > > + error_setg(errp, "error parsing host:port '%s'", > buf); > > > > + return NULL; > > > > + } > > > > } > > > > } > > > > > > > > - addr->host =3D g_strdup(host); > > > > - addr->port =3D g_strdup(port); > > > > + *hostp =3D g_strdup(host); > > > > + *portp =3D g_strdup(port); > > > > + *is_v6 =3D buf[0] =3D=3D '['; > > > > + > > > > + 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 =3D 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 =3D=3D NULL) { > > > > + return -1; > > > > + } > > > > > > > > /* parse options */ > > > > - optstr =3D str + pos; > > > > h =3D strstr(optstr, ",to=3D"); > > > > if (h) { > > > > h +=3D 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 l= ot > > 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. --00000000000047a67b05bc050580 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Mon, Feb 22, 2021 at 1:39 AM Daniel P. Berrang=C3=A9 <<= a href=3D"mailto:berrange@redhat.com">berrange@redhat.com> wrote:
On Fri, Feb 19, 2021 at 02:17:42PM -0800, Doug Evans wrote:<= br> > On Fri, Feb 19, 2021 at 2:00 AM Daniel P. Berrang=C3=A9 <berrange@redhat.com><= br> > 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_p= ort.
> > > This is done in preparation for using the function in net/sl= irp.c.
> > >
> > > Signed-off-by: Doug Evans <dje@google.com>
> > > ---
> > >
> > > Changes from v3:
> > > - this patch is new in v4
> > >=C2=A0 =C2=A0- provides new utility: inet_parse_host_and_port= , updates inet_parse
> > >=C2=A0 =C2=A0 =C2=A0to use it
> > >
> > >=C2=A0 include/qemu/sockets.h |=C2=A0 3 ++
> > >=C2=A0 util/qemu-sockets.c=C2=A0 =C2=A0 | 94 ++++++++++++++++= +++++++++++++++-----------
> > >=C2=A0 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);
> > >
> > >=C2=A0 int inet_ai_family_from_address(InetSocketAddress *add= r,
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Error **errp);<= br> > > > +const char* inet_parse_host_and_port(const char* str, int t= erminator,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ch= ar **addr, char **port, bool
> > *is_v6,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Er= ror **errp);
> > >=C2=A0 int inet_parse(InetSocketAddress *addr, const char *st= r, Error **errp);
> > >=C2=A0 int inet_connect(const char *str, Error **errp);
> > >=C2=A0 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,
> > >=C2=A0 =C2=A0 =C2=A0 return 0;
> > >=C2=A0 }
> > >
> > > -int inet_parse(InetSocketAddress *addr, const char *str, Er= ror **errp)
> > > +/*
> > > + * Parse an inet host and port as "host:port<termin= ator>".
> > > + * Terminator may be '\0'.
> > > + * The syntax for ipv4 addresses is: address:port.
> > > + * The syntax for ipv6 addresses is: [address]:port.
> >
> > It also supports
> >
> >=C2=A0 =C2=A0 "The syntax for hostnames is hostname:port
> >
> > > + * On success, returns a pointer to the terminator. Space f= or 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.<= br> > >
> > > + * On failure NULL is returned with the error stored in *er= rp.
> > > + */
> > > +const char* inet_parse_host_and_port(const char* str, int t= erminator,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ch= ar **hostp, char **portp, bool
> > *is_v6,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0Er= ror **errp)
> > >=C2=A0 {
> > > -=C2=A0 =C2=A0 const char *optstr, *h;
> > > +=C2=A0 =C2=A0 const char *terminator_ptr =3D strchr(str, te= rminator);
> > > +=C2=A0 =C2=A0 g_autofree char *buf =3D NULL;
> > >=C2=A0 =C2=A0 =C2=A0 char host[65];
> > >=C2=A0 =C2=A0 =C2=A0 char port[33];
> > > -=C2=A0 =C2=A0 int to;
> > > -=C2=A0 =C2=A0 int pos;
> > > -=C2=A0 =C2=A0 char *begin;
> > >
> > > -=C2=A0 =C2=A0 memset(addr, 0, sizeof(*addr));
> > > +=C2=A0 =C2=A0 if (terminator_ptr =3D=3D NULL) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* If the terminator isn't = found then use the entire string. */
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 terminator_ptr =3D str + strlen= (str);
> > > +=C2=A0 =C2=A0 }
> > > +=C2=A0 =C2=A0 buf =3D g_strndup(str, terminator_ptr - str);=
> > >
> > > -=C2=A0 =C2=A0 /* parse address */
> > > -=C2=A0 =C2=A0 if (str[0] =3D=3D ':') {
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* no host given */
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 host[0] =3D '\0';
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(str, ":%32[^,]%= n", port, &pos) !=3D 1) {
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp, = "error parsing port in address '%s'", str);
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> >
> >
> > > -=C2=A0 =C2=A0 } else if (str[0] =3D=3D '[') {
> > > +=C2=A0 =C2=A0 if (buf[0] =3D=3D '[') {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* IPv6 addr */
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(str, "[%64[^]]]= :%32[^,]%n", host, port, &pos) !=3D 2) {
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp, = "error parsing IPv6 address '%s'", str);
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (buf[1] =3D=3D ']') = {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* sscanf %[ does= n't recognize empty contents. */
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 host[0] =3D '= \0';
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(buf, &= quot;[]:%32s", port) !=3D 1) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err= or_setg(errp, "error parsing IPv6 host:port '%s'",
> > buf);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret= urn NULL;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> >
> > This is introducing new functionality to the parser. Current call= ers
> > let empty string ":port" be used for both ipv4 and ipv6= , based
> > on whether the flags ",ipv4[=3Don|off],ipv6[=3Don|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 ip= v4 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 th= e
> > existing ":port" exclusively means ipv4. IIC, this is a= lso why you
> > needed to introduce the "is_v6" flag, because any non-e= mpty address
> > can be reliably parsed without needing this flag.
> >
>
>
> Actually, no. The "is_v6" flag is needed because otherwise t= he caller has
> no means (other than maybe subsequent grepping for "." vs &q= uot;:") 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 consid= ering 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 "[]&qu= ot;, we should
> > not let that be parsed, unless the caller passing a "is_v6&q= uot; pointer
> > which is not NULL.
> >
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(buf, &= quot;[%64[^]]]:%32s", host, port) !=3D 2) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err= or_setg(errp, "error parsing IPv6 host:port '%s'",
> > buf);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret= urn NULL;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > >=C2=A0 =C2=A0 =C2=A0 } else {
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* hostname or IPv4 addr */
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(str, "%64[^:]:%= 32[^,]%n", host, port, &pos) !=3D 2) {
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp, = "error parsing address '%s'", str);
> > > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (buf[0] =3D=3D ':') = {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* no host given = */
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 host[0] =3D '= \0';
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(buf, &= quot;:%32s", port) !=3D 1) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err= or_setg(errp, "error parsing host:port '%s'", buf);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret= urn NULL;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> >
> > It would be preferreable if the parsing code was not re-ordered w= hen
> > extracting it. It doesn't look like a functional change, but = I'm unsure
> > why you moved it ?
> >
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 } else {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* hostname or IP= v4 addr */
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (sscanf(buf, &= quot;%64[^:]:%32s", host, port) !=3D 2) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err= or_setg(errp, "error parsing host:port '%s'", buf);
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret= urn NULL;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> > >=C2=A0 =C2=A0 =C2=A0 }
> > >
> > > -=C2=A0 =C2=A0 addr->host =3D g_strdup(host);
> > > -=C2=A0 =C2=A0 addr->port =3D g_strdup(port);
> > > +=C2=A0 =C2=A0 *hostp =3D g_strdup(host);
> > > +=C2=A0 =C2=A0 *portp =3D g_strdup(port);
> > > +=C2=A0 =C2=A0 *is_v6 =3D buf[0] =3D=3D '[';
> > > +
> > > +=C2=A0 =C2=A0 return terminator_ptr;
> > > +}
> > > +
> > > +int inet_parse(InetSocketAddress *addr, const char *str, Er= ror **errp)
> > > +{
> > > +=C2=A0 =C2=A0 const char *optstr, *h;
> > > +=C2=A0 =C2=A0 bool is_v6;
> > > +=C2=A0 =C2=A0 int to;
> > > +=C2=A0 =C2=A0 int pos;
> > > +=C2=A0 =C2=A0 char *begin;
> > > +
> > > +=C2=A0 =C2=A0 memset(addr, 0, sizeof(*addr));
> > > +
> > > +=C2=A0 =C2=A0 optstr =3D inet_parse_host_and_port(str, '= ;,', &addr->host,
> > &addr->port,
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 &= amp;is_v6, errp);
> >
> > Just pass NULL since we don't need is_v6
> >
> > > +=C2=A0 =C2=A0 if (optstr =3D=3D NULL) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return -1;
> > > +=C2=A0 =C2=A0 }
> > >
> > >=C2=A0 =C2=A0 =C2=A0 /* parse options */
> > > -=C2=A0 =C2=A0 optstr =3D str + pos;
> > >=C2=A0 =C2=A0 =C2=A0 h =3D strstr(optstr, ",to=3D")= ;
> > >=C2=A0 =C2=A0 =C2=A0 if (h) {
> > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 h +=3D 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 providin= g the
> caller with the means to distinguish whether str was "address:por= t" or
> "[address]:port" is a poor API.

In general callers shouldn't care about which format was parsed. The us= e
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 change= s. No point in continually sending revisions until then.
--00000000000047a67b05bc050580--