Re: [Qemu-devel] [PATCH 6/9] slirp: Factorizing and cleaning solookup()

[Thomas Huth <thuth@redhat.com>]

On 14/12/15 14:51, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> solookup() was only compatible with TCP. Having the socket list in
> argument, it is now compatible with UDP too.
> 
> Some optimization code is factorized inside the function (the function
> look at the last returned result before browsing the complete socket
> list).
> 
> 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    | 37 ++++++++++++++++++++++++-------------
>  slirp/socket.h    |  5 +++--
>  slirp/tcp_input.c | 13 +++----------
>  slirp/udp.c       | 21 ++-------------------
>  4 files changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/slirp/socket.c b/slirp/socket.c
> index d1034fb..8f73e90 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -16,23 +16,34 @@ static void sofcantrcvmore(struct socket *so);
>  static void sofcantsendmore(struct socket *so);
>  
>  struct socket *
> -solookup(struct socket *head, struct in_addr laddr, u_int lport,
> +solookup(struct socket **last, struct socket *head,
> +         struct in_addr laddr, u_int lport,
>           struct in_addr faddr, u_int fport)
>  {
> -	struct socket *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 &&
> -		    so->so_faddr.s_addr == faddr.s_addr &&
> -		    so->so_fport == fport)
> -		   break;
> -	}
> +    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))) {
> +        return so;
> +    }
>  
> -	if (so == head)
> -	   return (struct socket *)NULL;
> -	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))) {

I somewhat dislike the "!faddr.s_addr" check which is used to see
whether it's the call from UDP instead of TCP and thus whether the
foreign addr and port have to be checked ... this feels like it could
also go wrong sometimes (couldn't faddr.s_addr be zero in the TCP case
by accident?)...

Anyway, since this gets reworked in the next patch already, and the
check then looks fine afterwards, I think this should be enough here as
a temporary solution, so I'm also fine if this patch gets merged as it is.

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