Re: [Qemu-devel] [PATCH 4/9] slirp: Make Socket structure IPv6 compatible

[Thomas Huth <thuth@redhat.com>]

On 14/12/15 14:51, Samuel Thibault wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> This patch replaces foreign and local address/port couples in Socket
> structure by 2 sockaddr_storage which can be casted in sockaddr_in.
> Direct access to address and port is still possible thanks to some
> \#define, so retrocompatibility of the existing code is assured.
> 
> The ss_family field of sockaddr_storage is declared after each socket
> creation.
> 
> The whole structure is also saved/restored when a Qemu session is
> saved/restored.
> 
> This prepares for IPv6 support.
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/ip_icmp.c   |  2 ++
>  slirp/slirp.c     | 50 +++++++++++++++++++++++++++++++++++++++++---------
>  slirp/socket.c    | 14 +++++++++++---
>  slirp/socket.h    | 19 +++++++++++++++----
>  slirp/tcp_input.c |  2 ++
>  slirp/tcp_subr.c  |  2 ++
>  slirp/udp.c       |  4 ++++
>  7 files changed, 77 insertions(+), 16 deletions(-)
> 
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 23b9f0f..58b7ceb 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -170,8 +170,10 @@ icmp_input(struct mbuf *m, int hlen)
>  	goto end_error;
>        }
>        so->so_m = m;
> +      so->so_ffamily = AF_INET;
>        so->so_faddr = ip->ip_dst;
>        so->so_fport = htons(7);
> +      so->so_lfamily = AF_INET;
>        so->so_laddr = ip->ip_src;
>        so->so_lport = htons(9);
>        so->so_iptos = ip->ip_tos;
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index f8dc505..179af59 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -234,7 +234,7 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork,
>  
>      slirp->opaque = opaque;
>  
> -    register_savevm(NULL, "slirp", 0, 3,
> +    register_savevm(NULL, "slirp", 0, 4,
>                      slirp_state_save, slirp_state_load, slirp);
>  
>      QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);
> @@ -1046,10 +1046,26 @@ static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
>  static void slirp_socket_save(QEMUFile *f, struct socket *so)
>  {
>      qemu_put_be32(f, so->so_urgc);
> -    qemu_put_be32(f, so->so_faddr.s_addr);
> -    qemu_put_be32(f, so->so_laddr.s_addr);
> -    qemu_put_be16(f, so->so_fport);
> -    qemu_put_be16(f, so->so_lport);
> +    qemu_put_be16(f, so->so_ffamily);
> +    switch (so->so_ffamily) {
> +    case AF_INET:
> +        qemu_put_be32(f, so->so_faddr.s_addr);
> +        qemu_put_be16(f, so->so_fport);
> +        break;
> +    default:
> +        fprintf(stderr,
> +                "so_ffamily unknown, unable to save so_faddr and so_fport\n");
> +    }
> +    qemu_put_be16(f, so->so_lfamily);
> +    switch (so->so_lfamily) {
> +    case AF_INET:
> +        qemu_put_be32(f, so->so_laddr.s_addr);
> +        qemu_put_be16(f, so->so_lport);
> +        break;
> +    default:
> +        fprintf(stderr,
> +                "so_ffamily unknown, unable to save so_laddr and so_lport\n");
> +    }
>      qemu_put_byte(f, so->so_iptos);
>      qemu_put_byte(f, so->so_emu);
>      qemu_put_byte(f, so->so_type);
> @@ -1169,10 +1185,26 @@ static int slirp_socket_load(QEMUFile *f, struct socket *so)
>          return -ENOMEM;
>  
>      so->so_urgc = qemu_get_be32(f);
> -    so->so_faddr.s_addr = qemu_get_be32(f);
> -    so->so_laddr.s_addr = qemu_get_be32(f);
> -    so->so_fport = qemu_get_be16(f);
> -    so->so_lport = qemu_get_be16(f);
> +    so->so_ffamily = qemu_get_be16(f);
> +    switch (so->so_ffamily) {
> +    case AF_INET:
> +        so->so_faddr.s_addr = qemu_get_be32(f);
> +        so->so_fport = qemu_get_be16(f);
> +        break;
> +    default:
> +        fprintf(stderr,
> +                "so_ffamily unknown, unable to restore so_faddr and so_lport\n");
> +    }
> +    so->so_lfamily = qemu_get_be16(f);
> +    switch (so->so_lfamily) {
> +    case AF_INET:
> +        so->so_laddr.s_addr = qemu_get_be32(f);
> +        so->so_lport = qemu_get_be16(f);
> +        break;
> +    default:
> +        fprintf(stderr,
> +                "so_ffamily unknown, unable to restore so_laddr and so_lport\n");
> +    }
>      so->so_iptos = qemu_get_byte(f);
>      so->so_emu = qemu_get_byte(f);
>      so->so_type = qemu_get_byte(f);

Most of the other new QEMU sources rather use error_report() instead of
fprintf(stderr, ...) nowadays, so in case you rework this patch again,
you might want to consider that, too. (However, I am not fully sure
whether this is appropriate for the slirp code since it could still be
considered as a module that could be used independently of QEMU, too)

Anyway, patch looks fine to me now, so:

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