Re: [Qemu-devel] [PATCHv9 09/10] qapi-schema, qemu-options & slirp: Adding Qemu options for IPv6 addresses

[Thomas Huth <thuth@redhat.com>]

On 22.02.2016 20:28, Samuel Thibault wrote:
> From: Yann Bordenave <meow@meowstars.org>
> 
> This patch adds parameters to manage some new options in the qemu -net
> command.
> Slirp IPv6 address, network prefix, and DNS IPv6 address can be given in
> argument to the qemu command.
> Defaults parameters are respectively fec0::2, fec0::, /64 and fec0::3.
> 
> Signed-off-by: Yann Bordenave <meow@meowstars.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  net/net.c        | 31 +++++++++++++++++++++++++
>  net/slirp.c      | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  qapi-schema.json | 40 ++++++++++++++++++++------------
>  qemu-options.hx  | 18 +++++++++++++--
>  slirp/libslirp.h |  8 ++++---
>  slirp/slirp.c    | 16 +++++++------
>  6 files changed, 150 insertions(+), 32 deletions(-)
...
> diff --git a/net/slirp.c b/net/slirp.c
> index 6b51fbc..bd712bc 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -36,6 +36,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "slirp/libslirp.h"
> +#include "slirp/ip6.h"
>  #include "sysemu/char.h"
>  
>  static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
> @@ -135,10 +136,13 @@ static NetClientInfo net_slirp_info = {
>  static int net_slirp_init(NetClientState *peer, const char *model,
>                            const char *name, int restricted,
>                            const char *vnetwork, const char *vhost,
> +                          const char *vprefix6, int vprefix6_len,
> +                          const char *vhost6,
>                            const char *vhostname, const char *tftp_export,
>                            const char *bootfile, const char *vdhcp_start,
> -                          const char *vnameserver, const char *smb_export,
> -                          const char *vsmbserver, const char **dnssearch)
> +                          const char *vnameserver, const char *vnameserver6,
> +                          const char *smb_export, const char *vsmbserver,
> +                          const char **dnssearch)
>  {
>      /* default settings according to historic slirp */
>      struct in_addr net  = { .s_addr = htonl(0x0a000200) }; /* 10.0.2.0 */
> @@ -146,6 +150,9 @@ static int net_slirp_init(NetClientState *peer, const char *model,
>      struct in_addr host = { .s_addr = htonl(0x0a000202) }; /* 10.0.2.2 */
>      struct in_addr dhcp = { .s_addr = htonl(0x0a00020f) }; /* 10.0.2.15 */
>      struct in_addr dns  = { .s_addr = htonl(0x0a000203) }; /* 10.0.2.3 */
> +    struct in6_addr ip6_prefix;
> +    struct in6_addr ip6_host;
> +    struct in6_addr ip6_dns;
>  #ifndef _WIN32
>      struct in_addr smbsrv = { .s_addr = 0 };
>  #endif
> @@ -235,6 +242,52 @@ static int net_slirp_init(NetClientState *peer, const char *model,
>      }
>  #endif
>  
> +
> +    if (!vprefix6) {
> +        vprefix6 = "fec0::";
> +    }
> +    if (!inet_pton(AF_INET6, vprefix6, &ip6_prefix)) {
> +        return -1;
> +    }
> +
> +    if (!vprefix6_len) {
> +        vprefix6_len = 64;
> +    }
> +    if (vprefix6_len < 0 || vprefix6_len > 128) {
> +        return -1;
> +    }

I think you could also immediately check for vprefix6_len > 126 here
already, then you could drop the two checks for 126 below. A prefix that
is bigger than 126 does not make much sense anyway, so I think it should
be ok to always disallow this (especially since there will be at least
three hosts in this network - the host, the guest, and the DNS server).

> +    if (vhost6) {
> +        if (!inet_pton(AF_INET6, vhost6, &ip6_host)) {
> +            return -1;
> +        }
> +        if (!in6_equal_net(&ip6_prefix, &ip6_host, vprefix6_len)) {
> +            return -1;
> +        }
> +    } else {
> +        if (vprefix6_len > 126) {
> +            return -1;
> +        }
> +        ip6_host = ip6_prefix;
> +        ip6_host.s6_addr[15] |= 2;
> +    }
> +
> +    if (vnameserver6) {
> +        if (!inet_pton(AF_INET6, vnameserver6, &ip6_dns)) {
> +            return -1;
> +        }
> +        if (!in6_equal_net(&ip6_prefix, &ip6_dns, vprefix6_len)) {
> +            return -1;
> +        }
> +    } else {
> +        if (vprefix6_len > 126) {
> +            return -1;
> +        }
> +        ip6_dns = ip6_prefix;
> +        ip6_dns.s6_addr[15] |= 3;
> +    }

The other parts of the patch looks fine to me now.

 Thomas