diff mbox

[7/8] slirp: VMStatify socket level

Message ID 20161027153217.16984-8-dgilbert@redhat.com
State New
Headers show

Commit Message

Dr. David Alan Gilbert Oct. 27, 2016, 3:32 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  | 150 +++++++++++++++++++++++++++------------------------------
 slirp/socket.h |   6 +--
 2 files changed, 74 insertions(+), 82 deletions(-)

Comments

Samuel Thibault Oct. 30, 2016, 2:47 p.m. UTC | #1 
Hello,

Dr. David Alan Gilbert (git), on Thu 27 Oct 2016 16:32:16 +0100, wrote:
> -    case AF_INET:
> -        qemu_put_be32(f, so->so_faddr.s_addr);
> -        qemu_put_be16(f, so->so_fport);
> -        break;

> +    if (version_id >= 4 && !is_inet) {
> +        error_report("%s: so_ffamily unknown, socket not preserved", __func__);
>      }

Well, no, we need to settle this another way, because we want to be able
to easily add inet6 support here.  At least pave the way in a way that
makes it not unnecessarily hard.  The code you are adding here looks to
me like very hard to rework to make it support the various socket
families.

> +        VMSTATE_UINT16_V(so_ffamily, struct socket, 4),
> +        VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket,
> +                            slirp_v4_or_newer_ffamily_inet),
> +        VMSTATE_UINT16_TEST(so_fport, struct socket,
> +                            slirp_v4_or_newer_ffamily_inet),

Does VMStat not provide a way to have differing content depending on a
field? (here, so_ffamily)

Samuel
Dr. David Alan Gilbert Nov. 7, 2016, 7:55 p.m. UTC | #2 
* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Hello,
> 
> Dr. David Alan Gilbert (git), on Thu 27 Oct 2016 16:32:16 +0100, wrote:
> > -    case AF_INET:
> > -        qemu_put_be32(f, so->so_faddr.s_addr);
> > -        qemu_put_be16(f, so->so_fport);
> > -        break;
> 
> > +    if (version_id >= 4 && !is_inet) {
> > +        error_report("%s: so_ffamily unknown, socket not preserved", __func__);
> >      }
> 
> Well, no, we need to settle this another way, because we want to be able
> to easily add inet6 support here.  At least pave the way in a way that
> makes it not unnecessarily hard.  The code you are adding here looks to
> me like very hard to rework to make it support the various socket
> families.

Well, I was just trying to match the current semantics/errors there.

> > +        VMSTATE_UINT16_V(so_ffamily, struct socket, 4),
> > +        VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket,
> > +                            slirp_v4_or_newer_ffamily_inet),
> > +        VMSTATE_UINT16_TEST(so_fport, struct socket,
> > +                            slirp_v4_or_newer_ffamily_inet),
> 
> Does VMStat not provide a way to have differing content depending on a
> field? (here, so_ffamily)

It has two things:
   a) Versions (e.g. VMSTATE_UINT16_V) - that says only include this field if
      we're on version >= n
   b) Tests (.e.g. VMSTATE_UINT16_TEST) - that says only include the field
      if the test says so.  The tests are a bit tedious since they're each
      a function, their is no generic test description scheme.
      (A lambda here would be lovely, but making one to work on both Gcc and
      Clang and anything else isn't trivial; I looked)

I think the underlying macros would make it easy to do a VMSTATE_UINT16_TEST_V
that only happens if both the test and the check are true which might
make this easier.

It's not that hard, but a bit tedious if we wanted to add another family here;
we'd end up with:

        VMSTATE_UINT16_V(so_ffamily, struct socket, 4),
        VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket,
                            slirp_v4_or_newer_ffamily_inet),
        VMSTATE_UINT32_TEST(so_faddr.s_addr6, struct socket,
                            slirp_v4_or_newer_ffamily_inet6),
        VMSTATE_UINT16_TEST(so_fport, struct socket,
                            slirp_v4_or_newer_ffamily_inet),

and change the test to:
static bool slirp_v4_or_newer_ffamily_inet(void *opaque, int version_id)
{
    bool is_inet = ((struct socket *)opaque)->so_ffamily == AF_INET;
    bool is_inet6 = ((struct socket *)opaque)->so_ffamily == AF_INET6;
    if (version_id >= 4 && !is_inet && !is_inet6) {
        error_report("%s: so_ffamily unknown, socket not preserved", __func__);
    }
    return version_id >= 4 && is_inet;
}

static bool slirp_v4_or_newer_ffamily_inet6(void *opaque, int version_id)
{
    bool is_inet6 = ((struct socket *)opaque)->so_ffamily == AF_INET6;
    return version_id >= 4 && is_inet6;
}

The asymmetry is purely to generate the error.

Actually, I might be able to avoid some of the lfamily/ffamily
duplication here - I hadn't spotted the were macros for lhost.ss.ss_family,
so I think I can rework the v4 or newer by doing:

  a) Split the fhost and lhost union's out as a separate union and share
them.
  b) For the v4 entries do:
  VMSTATE_STRUCT(fhost, struct socket, 4, vmstate_slirp_hosts);
  VMSTATE_STRUCT(lhost, struct socket, 4, vmstate_slirp_hosts);

  and define vmstate_slirp_hosts to have the logic to check the family;
  although I've never tried VMSTATE_STRUCT on a union I think it'll work (!).

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Juan Quintela Feb. 13, 2017, 12:13 p.m. UTC | #3 
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>


> diff --git a/slirp/socket.h b/slirp/socket.h
> index 8feed2a..0137928 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -30,7 +30,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 {   /* foreign host */
>        struct sockaddr_storage ss;
>        struct sockaddr_in sin;
> @@ -56,8 +56,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 */

I normally preffer to fix types in a previous patch, but that is clearly
a question of taste.

Later, Juan.
diff mbox

Patch

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 2f7802e..d9532d6 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1250,40 +1250,81 @@  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_v4_or_newer_ffamily_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");
+    bool is_inet = ((struct socket *)opaque)->so_ffamily == AF_INET;
+    if (version_id >= 4 && !is_inet) {
+        error_report("%s: so_ffamily unknown, socket not preserved", __func__);
     }
-    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");
+    return version_id >= 4 && is_inet;
+}
+
+static bool slirp_v4_or_newer_lfamily_inet(void *opaque, int version_id)
+{
+    bool is_inet = ((struct socket *)opaque)->so_lfamily == AF_INET;
+    if (version_id >= 4 && !is_inet) {
+        error_report("%s: so_lfamily unknown, socket not preserved", __func__);
     }
-    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);
+    return version_id >= 4 && is_inet;
+}
+
+static int slirp_socket_pre_load(void *opaque)
+{
+    struct socket *so = opaque;
+    if (tcp_attach(so) < 0) {
+        return -ENOMEM;
+    }
+    /* 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 = {
+    .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 - note the order difference as well as extras */
+        VMSTATE_UINT16_V(so_ffamily, struct socket, 4),
+        VMSTATE_UINT32_TEST(so_faddr.s_addr, struct socket,
+                            slirp_v4_or_newer_ffamily_inet),
+        VMSTATE_UINT16_TEST(so_fport, struct socket,
+                            slirp_v4_or_newer_ffamily_inet),
+
+        VMSTATE_UINT16_V(so_lfamily, struct socket, 4),
+        VMSTATE_UINT32_TEST(so_laddr.s_addr, struct socket,
+                            slirp_v4_or_newer_lfamily_inet),
+        VMSTATE_UINT16_TEST(so_lport, struct socket,
+                            slirp_v4_or_newer_lfamily_inet),
+
+        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 +1349,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 +1358,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 +1381,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 8feed2a..0137928 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -30,7 +30,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 {   /* foreign host */
       struct sockaddr_storage ss;
       struct sockaddr_in sin;
@@ -56,8 +56,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 */