Re: [Qemu-devel] [PATCH 7/9] slirp: Add sockaddr_equal, make solookup family-agnostic

[Thomas Huth <thuth@redhat.com>]

On 14/12/15 14:51, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> This patch makes solookup() compatible with varying address
> families, by using a new sockaddr_equal() function that compares
> two sockaddr_storage.
> 
> This prepares for IPv6 support.
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/socket.c    | 21 ++++++---------------
>  slirp/socket.h    | 22 +++++++++++++++++++++-
>  slirp/tcp_input.c | 20 +++++++++++---------
>  slirp/udp.c       |  8 ++++++--
>  4 files changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index 8f73e90..a785b92 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -15,29 +15,20 @@
>  static void sofcantrcvmore(struct socket *so);
>  static void sofcantsendmore(struct socket *so);
>  
> -struct socket *
> -solookup(struct socket **last, struct socket *head,
> -         struct in_addr laddr, u_int lport,
> -         struct in_addr faddr, u_int fport)
> +struct socket *solookup(struct socket **last, struct socket *head,
> +        struct sockaddr_storage *lhost, struct sockaddr_storage *fhost)
>  {
>      struct socket *so = *last;
>  
>      /* Optimisation */
> -    if (so != head &&
> -            so->so_lport == lport &&
> -            so->so_laddr.s_addr == laddr.s_addr &&
> -            (!faddr.s_addr ||
> -                (so->so_faddr.s_addr == faddr.s_addr &&
> -                 so->so_fport == fport))) {
> +    if (so != head && sockaddr_equal(&(so->lhost.ss), lhost)
> +            && (!fhost || sockaddr_equal(&(so->fhost.ss), fhost))) {

I think you could omit the paranthesis around "so->fhost.ss" in above line.

>          return so;
>      }
>  
>      for (so = head->so_next; so != head; so = so->so_next) {
> -        if (so->so_lport == lport &&
> -            so->so_laddr.s_addr == laddr.s_addr &&
> -            (!faddr.s_addr ||
> -                (so->so_faddr.s_addr == faddr.s_addr &&
> -                 so->so_fport == fport))) {
> +        if (sockaddr_equal(&(so->lhost.ss), lhost)
> +                && (!fhost || sockaddr_equal(&(so->fhost.ss), fhost))) {

dito.

>              *last = so;
>              return so;
>          }
> diff --git a/slirp/socket.h b/slirp/socket.h
> index 1c8c24c..1331af6 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -87,8 +87,28 @@ struct socket {
>  #define SS_HOSTFWD		0x1000	/* Socket describes host->guest forwarding */
>  #define SS_INCOMING		0x2000	/* Connection was initiated by a host on the internet */
>  
> +static inline int sockaddr_equal(struct sockaddr_storage *a,
> +        struct sockaddr_storage *b)
> +{
> +    if (a->ss_family != b->ss_family) {
> +        return 0;
> +    }
> +
> +    switch (a->ss_family) {
> +    case AF_INET:
> +    {
> +        struct sockaddr_in *a4 = (struct sockaddr_in *) a;
> +        struct sockaddr_in *b4 = (struct sockaddr_in *) b;
> +        return (a4->sin_addr.s_addr == b4->sin_addr.s_addr
> +                && a4->sin_port == b4->sin_port);

You could omit the paranthesis around the check here.

> +    }
> +    default:
> +        assert(0);
> +    }
> +}
> +
>  struct socket *solookup(struct socket **, struct socket *,
> -        struct in_addr, u_int, struct in_addr, u_int);
> +        struct sockaddr_storage *, struct sockaddr_storage *);
>  struct socket *socreate(Slirp *);
>  void sofree(struct socket *);
>  int soread(struct socket *);
> diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
> index 5492061..8c4fa62 100644
> --- a/slirp/tcp_input.c
> +++ b/slirp/tcp_input.c
> @@ -227,6 +227,7 @@ tcp_input(struct mbuf *m, int iphlen, struct socket *inso)
>  	int iss = 0;
>  	u_long tiwin;
>  	int ret;
> +	struct sockaddr_storage lhost, fhost;
>      struct ex_list *ex_ptr;
>      Slirp *slirp;
>  
> @@ -320,9 +321,14 @@ tcp_input(struct mbuf *m, int iphlen, struct socket *inso)
>  	 * Locate pcb for segment.
>  	 */
>  findso:
> -	so = solookup(&slirp->tcp_last_so, &slirp->tcb,
> -		      ti->ti_src, ti->ti_sport,
> -		      ti->ti_dst, ti->ti_dport);
> +	lhost.ss_family = AF_INET;
> +	((struct sockaddr_in *)&lhost)->sin_addr = ti->ti_src;
> +	((struct sockaddr_in *)&lhost)->sin_port = ti->ti_sport;
> +	fhost.ss_family = AF_INET;
> +	((struct sockaddr_in *)&fhost)->sin_addr = ti->ti_dst;
> +	((struct sockaddr_in *)&fhost)->sin_port = ti->ti_dport;

Couldn't you simply use "fhost.sin.sin_addr = ..." etc. instead of
casting everything via a pointer?

> +	so = solookup(&slirp->tcp_last_so, &slirp->tcb, &lhost, &fhost);
>  
>  	/*
>  	 * If the state is CLOSED (i.e., TCB does not exist) then
> @@ -367,12 +373,8 @@ findso:
>  	  sbreserve(&so->so_snd, TCP_SNDSPACE);
>  	  sbreserve(&so->so_rcv, TCP_RCVSPACE);
>  
> -	  so->so_lfamily = AF_INET;
> -	  so->so_laddr = ti->ti_src;
> -	  so->so_lport = ti->ti_sport;
> -	  so->so_ffamily = AF_INET;
> -	  so->so_faddr = ti->ti_dst;
> -	  so->so_fport = ti->ti_dport;
> +	  so->lhost.ss = lhost;
> +	  so->fhost.ss = fhost;
>  
>  	  if ((so->so_iptos = tcp_tos(so)) == 0)
>  	    so->so_iptos = ((struct ip *)ti)->ip_tos;
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 126ef82..f2dd773 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -70,6 +70,7 @@ udp_input(register struct mbuf *m, int iphlen)
>  	int len;
>  	struct ip save_ip;
>  	struct socket *so;
> +	struct sockaddr_storage lhost;
>  
>  	DEBUG_CALL("udp_input");
>  	DEBUG_ARG("m = %p", m);
> @@ -151,8 +152,11 @@ udp_input(register struct mbuf *m, int iphlen)
>  	/*
>  	 * Locate pcb for datagram.
>  	 */
> -	so = solookup(&slirp->udp_last_so, &slirp->udb,
> -		      ip->ip_src, uh->uh_sport, (struct in_addr) {0}, 0);
> +	lhost.ss_family = AF_INET;
> +	((struct sockaddr_in *)&lhost)->sin_addr = ip->ip_src;
> +	((struct sockaddr_in *)&lhost)->sin_port = uh->uh_sport;

dito

> +	so = solookup(&slirp->udp_last_so, &slirp->udb, &lhost, NULL);
>  
>  	if (so == NULL) {
>  	  /*

Apart from the somewhat cumbersome typecasts (which are IMHO a little
bit ugly but still OK for me), the patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>