Re: [RFC PATCH 1/6] net: introduce convert_host_port()

[Laurent Vivier]

On 10/05/2022 23:24, Stefano Brivio wrote:
> On Mon,  9 May 2022 19:36:13 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
>
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > ---
> >   include/qemu/sockets.h |  2 ++
> >   net/net.c              | 62 ++++++++++++++++++++++--------------------
> >   2 files changed, 34 insertions(+), 30 deletions(-)
> >
> > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > index 038faa157f59..47194b9732f8 100644
> > --- a/include/qemu/sockets.h
> > +++ b/include/qemu/sockets.h
> > @@ -47,6 +47,8 @@ void socket_listen_cleanup(int fd, Error **errp);
> >   int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
> >   
> >   /* Old, ipv4 only bits.  Don't use for new code. */
> > +int convert_host_port(struct sockaddr_in *saddr, const char *host,
> > +                      const char *port, Error **errp);
> >   int parse_host_port(struct sockaddr_in *saddr, const char *str,
> >                       Error **errp);
> >   int socket_init(void);
> > diff --git a/net/net.c b/net/net.c
> > index a094cf1d2929..58c05c200622 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -66,55 +66,57 @@ static QTAILQ_HEAD(, NetClientState) net_clients;
> >   /***********************************************************/
> >   /* network device redirectors */
> >   
> > -int parse_host_port(struct sockaddr_in *saddr, const char *str,
> > -                    Error **errp)
> > +int convert_host_port(struct sockaddr_in *saddr, const char *host,
> > +                      const char *port, Error **errp)
> >   {
> > -    gchar **substrings;
> >       struct hostent *he;
> > -    const char *addr, *p, *r;
> > -    int port, ret = 0;
> > +    const char *r;
> > +    long p;
> >   
> >       memset(saddr, 0, sizeof(*saddr));
> >   
> > -    substrings = g_strsplit(str, ":", 2);
> > -    if (!substrings || !substrings[0] || !substrings[1]) {
> > -        error_setg(errp, "host address '%s' doesn't contain ':' "
> > -                   "separating host from port", str);
> > -        ret = -1;
> > -        goto out;
> > -    }
> > -
> > -    addr = substrings[0];
> > -    p = substrings[1];
> > -
> >       saddr->sin_family = AF_INET;
> > -    if (addr[0] == '\0') {
> > +    if (host[0] == '\0') {
> >           saddr->sin_addr.s_addr = 0;
> >       } else {
> > -        if (qemu_isdigit(addr[0])) {
> > -            if (!inet_aton(addr, &saddr->sin_addr)) {
> > +        if (qemu_isdigit(host[0])) {
> > +            if (!inet_aton(host, &saddr->sin_addr)) {
>
> I was about to observe that this doesn't support IPv6 addresses (which
> I guess we'd like to have always in the RFC 3986 form "[address]:port",
> as the port is mandatory here), and to propose a small change to bridge
> this gap.
>
> Then I realised this is (partially) using GLib, so maybe we want to use
> g_network_address_parse() which would, however, give us a
> GSocketConnectable object.
>
> At that point, do we want to pass the GsocketConnectable thing around,
> or stick to struct sockaddr_in{,6}, filling it here from what GLib
> gives us?

For the new netdev, we should better switch to util/qemu-sockets.c functions that take 
directly a SocketAddress (see socket_connect()).

Thanks,
Laurent