Re: [Qemu-devel] [PATCHv7 5/9] slirp: Generalizing and neutralizing various TCP functions before adding IPv6 stuff

[Thomas Huth]

On 14.02.2016 18:47, Samuel Thibault wrote:
> From: Guillaume Subiron <address@hidden>
> 
> Basically, this patch adds some switch in various TCP functions to
> prepare them for the IPv6 case.
> 
> To have something to "switch" in tcp_input() and tcp_respond(), a new
> argument is used to give them the sa_family of the addresses they are
> working on.
> 
> This patch does not include the entailed reindentation, to make proofread
> easier. Reindentation is adressed in the following no-op patch.
> 
> Signed-off-by: Guillaume Subiron <address@hidden>
> Signed-off-by: Samuel Thibault <address@hidden>
> ---
...
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index c2c4597..2321c41 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -574,7 +574,8 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>                          /*
>                           * Continue tcp_input
>                           */
> -                        tcp_input((struct mbuf *)NULL, sizeof(struct ip), 
> so);
> +                        tcp_input((struct mbuf *)NULL, sizeof(struct ip), so,
> +                                so->so_ffamily);

Cosmetic nit: indentation of "so->so_ffamily" should IMHO use two more
spaces.

>                          /* continue; */
>                      } else {
>                          ret = sowrite(so);
> @@ -623,7 +624,8 @@ void slirp_pollfds_poll(GArray *pollfds, int select_error)
>                          }
>  
>                      }
> -                    tcp_input((struct mbuf *)NULL, sizeof(struct ip), so);
> +                    tcp_input((struct mbuf *)NULL, sizeof(struct ip), so,
> +                            so->so_ffamily);

dito

>                  } /* SS_ISFCONNECTING */
>  #endif
>              }
...
> diff --git a/slirp/tcp_output.c b/slirp/tcp_output.c
> index 7fc6a87..1e5da73 100644
> --- a/slirp/tcp_output.c
> +++ b/slirp/tcp_output.c
> @@ -61,7 +61,8 @@ tcp_output(struct tcpcb *tp)
>       register long len, win;
>       int off, flags, error;
>       register struct mbuf *m;
> -     register struct tcpiphdr *ti;
> +     register struct tcpiphdr *ti, tcpiph_save;
> +     struct ip *ip;
>       u_char opt[MAX_TCPOPTLEN];
>       unsigned optlen, hdrlen;
>       int idle, sendalot;
> @@ -447,13 +448,15 @@ send:
>        * the template, but need a way to checksum without them.
>        */
>       m->m_len = hdrlen + len; /* XXX Needed? m_len should be correct */
> +     tcpiph_save = *(mtod(m, struct tcpiphdr *));

I think you could drop the outermost parentheses here.

> -     struct tcpiphdr tcpiph_save = *(mtod(m, struct tcpiphdr *));
> +     switch (so->so_ffamily) {
> +     case AF_INET:
>       m->m_data += sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
>                                            - sizeof(struct ip);
>       m->m_len  -= sizeof(struct tcpiphdr) - sizeof(struct tcphdr)
>                                            - sizeof(struct ip);
> -     struct ip *ip = mtod(m, struct ip *);
> +     ip = mtod(m, struct ip *);
>  
>       ip->ip_len = m->m_len;
>       ip->ip_dst = tcpiph_save.ti_dst;
> @@ -464,6 +467,11 @@ send:
>       ip->ip_tos = so->so_iptos;
>  
>       error = ip_output(so, m);
> +         break;
> +
> +     default:
> +         g_assert_not_reached();
> +     }
...

Only very minor cosmetic nits, patch generally looks fine, so:

Reviewed-by: Thomas Huth <address@hidden>