Re: [Qemu-devel] [PATCH 3/9] slirp: Adding address family switch for incoming frames

[Thomas Huth <thuth@redhat.com>]

On 14/12/15 14:51, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> In if_encap, a switch is added to prepare for the IPv6 case. Some code
> is factorized.
> 
> This prepares for IPv6 support.
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/slirp.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 48 insertions(+), 13 deletions(-)
> 
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index 1d5d172..f8dc505 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -762,20 +762,15 @@ void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len)
>      }
>  }
>  
> -/* Output the IP packet to the ethernet device. Returns 0 if the packet must be
> - * re-queued.
> +/* Prepare the IPv4 packet to be sent to the ethernet device. Returns 1 if no
> + * packet should be sent, 0 if the packet must be re-queued, 2 if the packet
> + * is ready to go.
>   */
> -int if_encap(Slirp *slirp, struct mbuf *ifm)
> +static int if_encap4(Slirp *slirp, struct mbuf *ifm, struct ethhdr *eh,
> +        uint8_t ethaddr[ETH_ALEN])
>  {
> -    uint8_t buf[1600];
> -    struct ethhdr *eh = (struct ethhdr *)buf;
> -    uint8_t ethaddr[ETH_ALEN];
>      const struct ip *iph = (const struct ip *)ifm->m_data;
>  
> -    if (ifm->m_len + ETH_HLEN > sizeof(buf)) {
> -        return 1;
> -    }
> -
>      if (iph->ip_dst.s_addr == 0) {
>          /* 0.0.0.0 can not be a destination address, something went wrong,
>           * avoid making it worse */
> @@ -819,15 +814,55 @@ int if_encap(Slirp *slirp, struct mbuf *ifm)
>          }
>          return 0;
>      } else {
> -        memcpy(eh->h_dest, ethaddr, ETH_ALEN);
>          memcpy(eh->h_source, special_ethaddr, ETH_ALEN - 4);
>          /* XXX: not correct */
>          memcpy(&eh->h_source[2], &slirp->vhost_addr, 4);
>          eh->h_proto = htons(ETH_P_IP);
> -        memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
> -        slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
> +
> +        /* Send this */
> +        return 2;
> +    }
> +}
> +
> +/* Output the IP packet to the ethernet device. Returns 0 if the packet must be
> + * re-queued.
> + */
> +int if_encap(Slirp *slirp, struct mbuf *ifm)
> +{
> +    uint8_t buf[1600];
> +    struct ethhdr *eh = (struct ethhdr *)buf;
> +    uint8_t ethaddr[ETH_ALEN];
> +    const struct ip *iph = (const struct ip *)ifm->m_data;
> +    int ret;
> +
> +    if (ifm->m_len + ETH_HLEN > sizeof(buf)) {
>          return 1;
>      }
> +
> +    switch (iph->ip_v) {
> +    case IPVERSION:
> +        ret = if_encap4(slirp, ifm, eh, ethaddr);
> +        if (ret < 2) {
> +            return ret;
> +        }
> +        break;
> +
> +    default:
> +        /* Do not assert while we don't manage IP6VERSION */
> +        /* assert(0); */

Not sure if we ever want to have an assert() here - since I assume this
could be triggered by the guest? In that case, it would be better to use
a qemu_log_mask(LOG_UNIMP, ...) or something similar in a later patch
instead and simply "return 1" afterwards.

> +        break;
> +    }
> +
> +    memcpy(eh->h_dest, ethaddr, ETH_ALEN);
> +    DEBUG_ARGS((dfd, " src = %02x:%02x:%02x:%02x:%02x:%02x\n",
> +                eh->h_source[0], eh->h_source[1], eh->h_source[2],
> +                eh->h_source[3], eh->h_source[4], eh->h_source[5]));
> +    DEBUG_ARGS((dfd, " dst = %02x:%02x:%02x:%02x:%02x:%02x\n",
> +                eh->h_dest[0], eh->h_dest[1], eh->h_dest[2],
> +                eh->h_dest[3], eh->h_dest[4], eh->h_dest[5]));
> +    memcpy(buf + sizeof(struct ethhdr), ifm->m_data, ifm->m_len);
> +    slirp_output(slirp->opaque, buf, ifm->m_len + ETH_HLEN);
> +    return 1;
>  }
>  
>  /* Drop host forwarding rule, return 0 if found. */

The patch now looks IMHO much nicer than the last version, thanks for
reworking it!

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