diff mbox

[v2,4/5] slirp: VMStatify socket level

Message ID 20161123185258.771-5-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Nov. 23, 2016, 6:52 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Working up the stack, this replaces the slirp_socket_load/save
with VMState definitions.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 slirp/slirp.c  | 146 ++++++++++++++++++++++++++-------------------------------
 slirp/socket.h |   6 +--
 2 files changed, 69 insertions(+), 83 deletions(-)

Comments

Samuel Thibault Nov. 27, 2016, 3:13 p.m. UTC | #1 
Hello,

Dr. David Alan Gilbert (git), on Wed 23 Nov 2016 18:52:57 +0000, wrote:
> +static const VMStateDescription vmstate_slirp_socket_addr = {
> +    .name = "slirp-socket-addr",
> +    .version_id = 4,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> +        VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> +                            slirp_family_inet),
> +        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> +                            slirp_family_inet),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

How will we be able to add the IPv6 case here?

Samuel
Samuel Thibault Nov. 27, 2016, 3:28 p.m. UTC | #2 
Samuel Thibault, on Sun 27 Nov 2016 16:13:46 +0100, wrote:
> Dr. David Alan Gilbert (git), on Wed 23 Nov 2016 18:52:57 +0000, wrote:
> > +static const VMStateDescription vmstate_slirp_socket_addr = {
> > +    .name = "slirp-socket-addr",
> > +    .version_id = 4,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> > +        VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> > +                            slirp_family_inet),
> > +        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> > +                            slirp_family_inet),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> 
> How will we be able to add the IPv6 case here?

Reading again your previous post, it seemed it'd be in
slirp_family_inet, but I don't immediately see how.

Applying your patch for 2.9 would thus make porting the code to IPv6
more difficult than how it is now, so I'm quite reluctant :)

Could you perhaps simply add the IPv6 case in your patch series already?
It shouldn't be much work for you who actually know how the VMSTATE
machinery is supposed to work (I guess the amount of people who care
about slirp *and* know about VMSTATE is extremely small), and a proof of
concept for the portability to non-ipv4 addresse spaces.

Samuel
Dr. David Alan Gilbert Nov. 28, 2016, 9:08 a.m. UTC | #3 
* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Samuel Thibault, on Sun 27 Nov 2016 16:13:46 +0100, wrote:
> > Dr. David Alan Gilbert (git), on Wed 23 Nov 2016 18:52:57 +0000, wrote:
> > > +static const VMStateDescription vmstate_slirp_socket_addr = {
> > > +    .name = "slirp-socket-addr",
> > > +    .version_id = 4,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> > > +        VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> > > +                            slirp_family_inet),
> > > +        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> > > +                            slirp_family_inet),
> > > +        VMSTATE_END_OF_LIST()
> > > +    }
> > > +};
> > 
> > How will we be able to add the IPv6 case here?
> 
> Reading again your previous post, it seemed it'd be in
> slirp_family_inet, but I don't immediately see how.
> 
> Applying your patch for 2.9 would thus make porting the code to IPv6
> more difficult than how it is now, so I'm quite reluctant :)
> 
> Could you perhaps simply add the IPv6 case in your patch series already?
> It shouldn't be much work for you who actually know how the VMSTATE
> machinery is supposed to work (I guess the amount of people who care
> about slirp *and* know about VMSTATE is extremely small), and a proof of
> concept for the portability to non-ipv4 addresse spaces.

The number of people who care about slirp, IPv6, VMState is even smaller :-)
Hmm, I don't really know IPv6 but I'm thinking this code will become something like
the following (says he not knowing whether a scope-id or a flowinfo
is something that needs migrating) (untested):


static const VMStateDescription vmstate_slirp_socket_addr = {
    .name = "slirp-socket-addr",
    .version_id = 4,
    .fields = (VMStateField[]) {
        VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
        VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
                            slirp_family_inet),
        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
                            slirp_family_inet),

        VMSTATE_BUFFER_TEST(sin6.sin6_addr, union slirp_sockaddr,
                            slirp_family_inet6),
        VMSTATE_UINT16_TEST(sin6.sin6_port, union slirp_sockaddr,
                            slirp_family_inet6),
        VMSTATE_UINT32_TEST(sin6.sin6_flowinfo, union slirp_sockaddr,
                            slirp_family_inet6),
        VMSTATE_UINT32_TEST(sin6.sin6_scope_id, union slirp_sockaddr,
                            slirp_family_inet6),
                            
        VMSTATE_END_OF_LIST()
    }
};

So to me that looks pretty clean, we need to add another slirp_family_inet6
test function, but then we just add the extra fields for the IPv6 stuff.

Can you suggest an IPv6 command line for testing that ?

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Samuel Thibault Nov. 28, 2016, 10:48 a.m. UTC | #4 
Hello,

Dr. David Alan Gilbert, on Mon 28 Nov 2016 09:08:16 +0000, wrote:
> Hmm, I don't really know IPv6 but I'm thinking this code will become something like
> the following (says he not knowing whether a scope-id or a flowinfo
> is something that needs migrating) (untested):
> 
> 
> static const VMStateDescription vmstate_slirp_socket_addr = {
>     .name = "slirp-socket-addr",
>     .version_id = 4,
>     .fields = (VMStateField[]) {
>         VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
>         VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
>                             slirp_family_inet),
>         VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
>                             slirp_family_inet),
> 
>         VMSTATE_BUFFER_TEST(sin6.sin6_addr, union slirp_sockaddr,
>                             slirp_family_inet6),
>         VMSTATE_UINT16_TEST(sin6.sin6_port, union slirp_sockaddr,
>                             slirp_family_inet6),
>         VMSTATE_UINT32_TEST(sin6.sin6_flowinfo, union slirp_sockaddr,
>                             slirp_family_inet6),
>         VMSTATE_UINT32_TEST(sin6.sin6_scope_id, union slirp_sockaddr,
>                             slirp_family_inet6),
>                             
>         VMSTATE_END_OF_LIST()
>     }
> };

Ok, that looks good :)

> So to me that looks pretty clean, we need to add another slirp_family_inet6
> test function, but then we just add the extra fields for the IPv6 stuff.

Yes, now I see.

> Can you suggest an IPv6 command line for testing that ?

Well, it doesn't exist yet, that's the problem. And applying your patch
would have made it to exist even harder, so that's why I was afraid.

I would say that your patch should contain these IPv6 lines, but as
comments since they are untested, for the person who will at some point
want to implement the IPv6 case here.

Samuel
Dr. David Alan Gilbert Nov. 30, 2016, 11:54 a.m. UTC | #5 
* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Hello,
> 
> Dr. David Alan Gilbert, on Mon 28 Nov 2016 09:08:16 +0000, wrote:
> > Hmm, I don't really know IPv6 but I'm thinking this code will become something like
> > the following (says he not knowing whether a scope-id or a flowinfo
> > is something that needs migrating) (untested):
> > 
> > 
> > static const VMStateDescription vmstate_slirp_socket_addr = {
> >     .name = "slirp-socket-addr",
> >     .version_id = 4,
> >     .fields = (VMStateField[]) {
> >         VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
> >         VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
> >                             slirp_family_inet),
> >         VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
> >                             slirp_family_inet),
> > 
> >         VMSTATE_BUFFER_TEST(sin6.sin6_addr, union slirp_sockaddr,
> >                             slirp_family_inet6),
> >         VMSTATE_UINT16_TEST(sin6.sin6_port, union slirp_sockaddr,
> >                             slirp_family_inet6),
> >         VMSTATE_UINT32_TEST(sin6.sin6_flowinfo, union slirp_sockaddr,
> >                             slirp_family_inet6),
> >         VMSTATE_UINT32_TEST(sin6.sin6_scope_id, union slirp_sockaddr,
> >                             slirp_family_inet6),
> >                             
> >         VMSTATE_END_OF_LIST()
> >     }
> > };
> 
> Ok, that looks good :)
> 
> > So to me that looks pretty clean, we need to add another slirp_family_inet6
> > test function, but then we just add the extra fields for the IPv6 stuff.
> 
> Yes, now I see.
> 
> > Can you suggest an IPv6 command line for testing that ?
> 
> Well, it doesn't exist yet, that's the problem. And applying your patch
> would have made it to exist even harder, so that's why I was afraid.
> 
> I would say that your patch should contain these IPv6 lines, but as
> comments since they are untested, for the person who will at some point
> want to implement the IPv6 case here.

OK, yes I can just add them as a comment and let someone who fixes
the rest of the IPv6 code turn it on.

Dave

> Samuel
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox

Patch

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 2f7802e..c631338 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1250,40 +1250,75 @@  static const VMStateDescription vmstate_slirp_sbuf = {
     }
 };
 
+static bool slirp_older_than_v4(void *opaque, int version_id)
+{
+    return version_id < 4;
+}
 
-static void slirp_socket_save(QEMUFile *f, struct socket *so)
+static bool slirp_family_inet(void *opaque, int version_id)
 {
-    qemu_put_be32(f, so->so_urgc);
-    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:
-        error_report("so_ffamily unknown, unable to save so_faddr and"
-                     " so_fport");
-    }
-    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:
-        error_report("so_ffamily unknown, unable to save so_laddr and"
-                     " so_lport");
+    union slirp_sockaddr *ssa = (union slirp_sockaddr *)opaque;
+    return ssa->ss.ss_family == AF_INET;
+}
+
+static int slirp_socket_pre_load(void *opaque)
+{
+    struct socket *so = opaque;
+    if (tcp_attach(so) < 0) {
+        return -ENOMEM;
     }
-    qemu_put_byte(f, so->so_iptos);
-    qemu_put_byte(f, so->so_emu);
-    qemu_put_byte(f, so->so_type);
-    qemu_put_be32(f, so->so_state);
-    /* TODO: Build vmstate at this level */
-    vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
-    vmstate_save_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
-    vmstate_save_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
+    /* Older versions don't load these fields */
+    so->so_ffamily = AF_INET;
+    so->so_lfamily = AF_INET;
+    return 0;
 }
 
+static const VMStateDescription vmstate_slirp_socket_addr = {
+    .name = "slirp-socket-addr",
+    .version_id = 4,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(ss.ss_family, union slirp_sockaddr),
+        VMSTATE_UINT32_TEST(sin.sin_addr.s_addr, union slirp_sockaddr,
+                            slirp_family_inet),
+        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
+                            slirp_family_inet),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_slirp_socket = {
+    .name = "slirp-socket",
+    .version_id = 4,
+    .pre_load = slirp_socket_pre_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(so_urgc, struct socket),
+        /* Pre-v4 versions */
+        VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket,
+                            slirp_older_than_v4),
+        VMSTATE_UINT32_TEST(so_laddr.s_addr, struct socket,
+                            slirp_older_than_v4),
+        VMSTATE_UINT16_TEST(so_fport, struct socket, slirp_older_than_v4),
+        VMSTATE_UINT16_TEST(so_lport, struct socket, slirp_older_than_v4),
+        /* v4 and newer */
+        VMSTATE_STRUCT(fhost, struct socket, 4, vmstate_slirp_socket_addr,
+                       union slirp_sockaddr),
+        VMSTATE_STRUCT(lhost, struct socket, 4, vmstate_slirp_socket_addr,
+                       union slirp_sockaddr),
+
+        VMSTATE_UINT8(so_iptos, struct socket),
+        VMSTATE_UINT8(so_emu, struct socket),
+        VMSTATE_UINT8(so_type, struct socket),
+        VMSTATE_INT32(so_state, struct socket),
+        VMSTATE_STRUCT(so_rcv, struct socket, 0, vmstate_slirp_sbuf,
+                       struct sbuf),
+        VMSTATE_STRUCT(so_snd, struct socket, 0, vmstate_slirp_sbuf,
+                       struct sbuf),
+        VMSTATE_STRUCT_POINTER(so_tcpcb, struct socket, vmstate_slirp_tcp,
+                       struct tcpcb),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void slirp_bootp_save(QEMUFile *f, Slirp *slirp)
 {
     int i;
@@ -1308,7 +1343,7 @@  static void slirp_state_save(QEMUFile *f, void *opaque)
                 continue;
 
             qemu_put_byte(f, 42);
-            slirp_socket_save(f, so);
+            vmstate_save_state(f, &vmstate_slirp_socket, so, NULL);
         }
     qemu_put_byte(f, 0);
 
@@ -1317,55 +1352,6 @@  static void slirp_state_save(QEMUFile *f, void *opaque)
     slirp_bootp_save(f, slirp);
 }
 
-static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
-{
-    int ret = 0;
-    if (tcp_attach(so) < 0)
-        return -ENOMEM;
-
-    so->so_urgc = qemu_get_be32(f);
-    if (version_id <= 3) {
-        so->so_ffamily = AF_INET;
-        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);
-    } else {
-        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:
-            error_report(
-                "so_ffamily unknown, unable to restore so_faddr and so_lport");
-        }
-        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:
-            error_report(
-                "so_ffamily unknown, unable to restore so_laddr and so_lport");
-        }
-    }
-    so->so_iptos = qemu_get_byte(f);
-    so->so_emu = qemu_get_byte(f);
-    so->so_type = qemu_get_byte(f);
-    so->so_state = qemu_get_be32(f);
-    /* TODO: VMState at this level */
-    ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_rcv, 0);
-    if (!ret) {
-        ret = vmstate_load_state(f, &vmstate_slirp_sbuf, &so->so_snd, 0);
-    }
-    if (!ret) {
-        ret = vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
-    }
-    return ret;
-}
 
 static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
 {
@@ -1389,7 +1375,7 @@  static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
         if (!so)
             return -ENOMEM;
 
-        ret = slirp_socket_load(f, so, version_id);
+        ret = vmstate_load_state(f, &vmstate_slirp_socket, so, version_id);
 
         if (ret < 0)
             return ret;
diff --git a/slirp/socket.h b/slirp/socket.h
index c1be77e..2f224bc 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -36,7 +36,7 @@  struct socket {
 				    * PING reply's */
   struct tcpiphdr *so_ti;	   /* Pointer to the original ti within
 				    * so_mconn, for non-blocking connections */
-  int so_urgc;
+  uint32_t      so_urgc;
   union slirp_sockaddr fhost;      /* Foreign host */
 #define so_faddr fhost.sin.sin_addr
 #define so_fport fhost.sin.sin_port
@@ -54,8 +54,8 @@  struct socket {
   uint8_t	so_iptos;	/* Type of service */
   uint8_t	so_emu;		/* Is the socket emulated? */
 
-  u_char	so_type;		/* Type of socket, UDP or TCP */
-  int	so_state;		/* internal state flags SS_*, below */
+  uint8_t       so_type;        /* Type of socket, UDP or TCP */
+  int32_t       so_state;       /* internal state flags SS_*, below */
 
   struct 	tcpcb *so_tcpcb;	/* pointer to TCP protocol control block */
   u_int	so_expire;		/* When the socket will expire */