[Qemu-devel] [PATCH v4 0/5] SLIRP VMStatification

[Qemu-devel] [PATCH v4 0/5] SLIRP VMStatification

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>


Hi,
  This is an update to my previous SLIRP vmstatification, but finally
the dependent patches have gone in, so this can now follow.


Dave

v4
  Fix 'VMStatify socket level' for mingw64 build; it has some different
  choices for the type of IPv4 addresses.

Dr. David Alan Gilbert (5):
  slirp: VMState conversion; tcpcb
  slirp: VMStatify sbuf
  slirp: Common lhost/fhost union
  slirp: VMStatify socket level
  slirp: VMStatify remaining except for loop

 slirp/sbuf.h    |   4 +-
 slirp/slirp.c   | 449 ++++++++++++++++++++++++++++----------------------------
 slirp/socket.h  |  24 ++-
 slirp/tcp_var.h |   6 +-
 4 files changed, 238 insertions(+), 245 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v4 1/5] slirp: VMState conversion; tcpcb

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Convert the migration of the struct tcpcb to use a VMStateDescription,
the rest of it will come later.

Mostly mechanical, except for conversion of some 'char' to uint8_t
to ensure portability.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 slirp/slirp.c   | 149 ++++++++++++++++++++------------------------------------
 slirp/tcp_var.h |   6 +--
 2 files changed, 57 insertions(+), 98 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 60539de..276d8cb 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1129,53 +1129,62 @@ void slirp_socket_recv(Slirp *slirp, struct in_addr guest_addr, int guest_port,
         tcp_output(sototcpcb(so));
 }
 
-static void slirp_tcp_save(QEMUFile *f, struct tcpcb *tp)
+static int slirp_tcp_post_load(void *opaque, int version)
 {
-    int i;
+    tcp_template((struct tcpcb *)opaque);
 
-    qemu_put_sbe16(f, tp->t_state);
-    for (i = 0; i < TCPT_NTIMERS; i++)
-        qemu_put_sbe16(f, tp->t_timer[i]);
-    qemu_put_sbe16(f, tp->t_rxtshift);
-    qemu_put_sbe16(f, tp->t_rxtcur);
-    qemu_put_sbe16(f, tp->t_dupacks);
-    qemu_put_be16(f, tp->t_maxseg);
-    qemu_put_sbyte(f, tp->t_force);
-    qemu_put_be16(f, tp->t_flags);
-    qemu_put_be32(f, tp->snd_una);
-    qemu_put_be32(f, tp->snd_nxt);
-    qemu_put_be32(f, tp->snd_up);
-    qemu_put_be32(f, tp->snd_wl1);
-    qemu_put_be32(f, tp->snd_wl2);
-    qemu_put_be32(f, tp->iss);
-    qemu_put_be32(f, tp->snd_wnd);
-    qemu_put_be32(f, tp->rcv_wnd);
-    qemu_put_be32(f, tp->rcv_nxt);
-    qemu_put_be32(f, tp->rcv_up);
-    qemu_put_be32(f, tp->irs);
-    qemu_put_be32(f, tp->rcv_adv);
-    qemu_put_be32(f, tp->snd_max);
-    qemu_put_be32(f, tp->snd_cwnd);
-    qemu_put_be32(f, tp->snd_ssthresh);
-    qemu_put_sbe16(f, tp->t_idle);
-    qemu_put_sbe16(f, tp->t_rtt);
-    qemu_put_be32(f, tp->t_rtseq);
-    qemu_put_sbe16(f, tp->t_srtt);
-    qemu_put_sbe16(f, tp->t_rttvar);
-    qemu_put_be16(f, tp->t_rttmin);
-    qemu_put_be32(f, tp->max_sndwnd);
-    qemu_put_byte(f, tp->t_oobflags);
-    qemu_put_byte(f, tp->t_iobc);
-    qemu_put_sbe16(f, tp->t_softerror);
-    qemu_put_byte(f, tp->snd_scale);
-    qemu_put_byte(f, tp->rcv_scale);
-    qemu_put_byte(f, tp->request_r_scale);
-    qemu_put_byte(f, tp->requested_s_scale);
-    qemu_put_be32(f, tp->ts_recent);
-    qemu_put_be32(f, tp->ts_recent_age);
-    qemu_put_be32(f, tp->last_ack_sent);
+    return 0;
 }
 
+static const VMStateDescription vmstate_slirp_tcp = {
+    .name = "slirp-tcp",
+    .version_id = 0,
+    .post_load = slirp_tcp_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT16(t_state, struct tcpcb),
+        VMSTATE_INT16_ARRAY(t_timer, struct tcpcb, TCPT_NTIMERS),
+        VMSTATE_INT16(t_rxtshift, struct tcpcb),
+        VMSTATE_INT16(t_rxtcur, struct tcpcb),
+        VMSTATE_INT16(t_dupacks, struct tcpcb),
+        VMSTATE_UINT16(t_maxseg, struct tcpcb),
+        VMSTATE_UINT8(t_force, struct tcpcb),
+        VMSTATE_UINT16(t_flags, struct tcpcb),
+        VMSTATE_UINT32(snd_una, struct tcpcb),
+        VMSTATE_UINT32(snd_nxt, struct tcpcb),
+        VMSTATE_UINT32(snd_up, struct tcpcb),
+        VMSTATE_UINT32(snd_wl1, struct tcpcb),
+        VMSTATE_UINT32(snd_wl2, struct tcpcb),
+        VMSTATE_UINT32(iss, struct tcpcb),
+        VMSTATE_UINT32(snd_wnd, struct tcpcb),
+        VMSTATE_UINT32(rcv_wnd, struct tcpcb),
+        VMSTATE_UINT32(rcv_nxt, struct tcpcb),
+        VMSTATE_UINT32(rcv_up, struct tcpcb),
+        VMSTATE_UINT32(irs, struct tcpcb),
+        VMSTATE_UINT32(rcv_adv, struct tcpcb),
+        VMSTATE_UINT32(snd_max, struct tcpcb),
+        VMSTATE_UINT32(snd_cwnd, struct tcpcb),
+        VMSTATE_UINT32(snd_ssthresh, struct tcpcb),
+        VMSTATE_INT16(t_idle, struct tcpcb),
+        VMSTATE_INT16(t_rtt, struct tcpcb),
+        VMSTATE_UINT32(t_rtseq, struct tcpcb),
+        VMSTATE_INT16(t_srtt, struct tcpcb),
+        VMSTATE_INT16(t_rttvar, struct tcpcb),
+        VMSTATE_UINT16(t_rttmin, struct tcpcb),
+        VMSTATE_UINT32(max_sndwnd, struct tcpcb),
+        VMSTATE_UINT8(t_oobflags, struct tcpcb),
+        VMSTATE_UINT8(t_iobc, struct tcpcb),
+        VMSTATE_INT16(t_softerror, struct tcpcb),
+        VMSTATE_UINT8(snd_scale, struct tcpcb),
+        VMSTATE_UINT8(rcv_scale, struct tcpcb),
+        VMSTATE_UINT8(request_r_scale, struct tcpcb),
+        VMSTATE_UINT8(requested_s_scale, struct tcpcb),
+        VMSTATE_UINT32(ts_recent, struct tcpcb),
+        VMSTATE_UINT32(ts_recent_age, struct tcpcb),
+        VMSTATE_UINT32(last_ack_sent, struct tcpcb),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
 {
     uint32_t off;
@@ -1218,7 +1227,7 @@ static void slirp_socket_save(QEMUFile *f, struct socket *so)
     qemu_put_be32(f, so->so_state);
     slirp_sbuf_save(f, &so->so_rcv);
     slirp_sbuf_save(f, &so->so_snd);
-    slirp_tcp_save(f, so->so_tcpcb);
+    vmstate_save_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
 }
 
 static void slirp_bootp_save(QEMUFile *f, Slirp *slirp)
@@ -1254,54 +1263,6 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
     slirp_bootp_save(f, slirp);
 }
 
-static void slirp_tcp_load(QEMUFile *f, struct tcpcb *tp)
-{
-    int i;
-
-    tp->t_state = qemu_get_sbe16(f);
-    for (i = 0; i < TCPT_NTIMERS; i++)
-        tp->t_timer[i] = qemu_get_sbe16(f);
-    tp->t_rxtshift = qemu_get_sbe16(f);
-    tp->t_rxtcur = qemu_get_sbe16(f);
-    tp->t_dupacks = qemu_get_sbe16(f);
-    tp->t_maxseg = qemu_get_be16(f);
-    tp->t_force = qemu_get_sbyte(f);
-    tp->t_flags = qemu_get_be16(f);
-    tp->snd_una = qemu_get_be32(f);
-    tp->snd_nxt = qemu_get_be32(f);
-    tp->snd_up = qemu_get_be32(f);
-    tp->snd_wl1 = qemu_get_be32(f);
-    tp->snd_wl2 = qemu_get_be32(f);
-    tp->iss = qemu_get_be32(f);
-    tp->snd_wnd = qemu_get_be32(f);
-    tp->rcv_wnd = qemu_get_be32(f);
-    tp->rcv_nxt = qemu_get_be32(f);
-    tp->rcv_up = qemu_get_be32(f);
-    tp->irs = qemu_get_be32(f);
-    tp->rcv_adv = qemu_get_be32(f);
-    tp->snd_max = qemu_get_be32(f);
-    tp->snd_cwnd = qemu_get_be32(f);
-    tp->snd_ssthresh = qemu_get_be32(f);
-    tp->t_idle = qemu_get_sbe16(f);
-    tp->t_rtt = qemu_get_sbe16(f);
-    tp->t_rtseq = qemu_get_be32(f);
-    tp->t_srtt = qemu_get_sbe16(f);
-    tp->t_rttvar = qemu_get_sbe16(f);
-    tp->t_rttmin = qemu_get_be16(f);
-    tp->max_sndwnd = qemu_get_be32(f);
-    tp->t_oobflags = qemu_get_byte(f);
-    tp->t_iobc = qemu_get_byte(f);
-    tp->t_softerror = qemu_get_sbe16(f);
-    tp->snd_scale = qemu_get_byte(f);
-    tp->rcv_scale = qemu_get_byte(f);
-    tp->request_r_scale = qemu_get_byte(f);
-    tp->requested_s_scale = qemu_get_byte(f);
-    tp->ts_recent = qemu_get_be32(f);
-    tp->ts_recent_age = qemu_get_be32(f);
-    tp->last_ack_sent = qemu_get_be32(f);
-    tcp_template(tp);
-}
-
 static int slirp_sbuf_load(QEMUFile *f, struct sbuf *sbuf)
 {
     uint32_t off, sb_cc, sb_datalen;
@@ -1367,9 +1328,7 @@ static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
         return -ENOMEM;
     if (slirp_sbuf_load(f, &so->so_snd) < 0)
         return -ENOMEM;
-    slirp_tcp_load(f, so->so_tcpcb);
-
-    return 0;
+    return vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
 }
 
 static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
diff --git a/slirp/tcp_var.h b/slirp/tcp_var.h
index 0f8f187..895ef6d 100644
--- a/slirp/tcp_var.h
+++ b/slirp/tcp_var.h
@@ -48,7 +48,7 @@ struct tcpcb {
 	short	t_rxtcur;		/* current retransmit value */
 	short	t_dupacks;		/* consecutive dup acks recd */
 	u_short	t_maxseg;		/* maximum segment size */
-	char	t_force;		/* 1 if forcing out a byte */
+	uint8_t t_force;		/* 1 if forcing out a byte */
 	u_short	t_flags;
 #define	TF_ACKNOW	0x0001		/* ack peer immediately */
 #define	TF_DELACK	0x0002		/* ack, but try to delay it */
@@ -109,8 +109,8 @@ struct tcpcb {
 	uint32_t max_sndwnd;		/* largest window peer has offered */
 
 /* out-of-band data */
-	char	t_oobflags;		/* have some */
-	char	t_iobc;			/* input character */
+	uint8_t	t_oobflags;		/* have some */
+	uint8_t	t_iobc;			/* input character */
 #define	TCPOOB_HAVEDATA	0x01
 #define	TCPOOB_HADDATA	0x02
 	short	t_softerror;		/* possible error not yet reported */
-- 
2.9.3

[Qemu-devel] [PATCH v4 2/5] slirp: VMStatify sbuf

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Convert the sbuf structure to a VMStateDescription.
Note this uses the VMSTATE_WITH_TMP mechanism to calculate
and reload the offsets based on the pointers.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 slirp/sbuf.h  |   4 +-
 slirp/slirp.c | 116 ++++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 78 insertions(+), 42 deletions(-)

diff --git a/slirp/sbuf.h b/slirp/sbuf.h
index efcec39..a722ecb 100644
--- a/slirp/sbuf.h
+++ b/slirp/sbuf.h
@@ -12,8 +12,8 @@
 #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
 
 struct sbuf {
-	u_int	sb_cc;		/* actual chars in buffer */
-	u_int	sb_datalen;	/* Length of data  */
+	uint32_t sb_cc;		/* actual chars in buffer */
+	uint32_t sb_datalen;	/* Length of data  */
 	char	*sb_wptr;	/* write pointer. points to where the next
 				 * bytes should be written in the sbuf */
 	char	*sb_rptr;	/* read pointer. points to where the next
diff --git a/slirp/slirp.c b/slirp/slirp.c
index 276d8cb..178c2b6 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp = {
     }
 };
 
-static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
+/* The sbuf has a pair of pointers that are migrated as offsets;
+ * we calculate the offsets and restore the pointers using
+ * pre_save/post_load on a tmp structure.
+ */
+struct sbuf_tmp {
+    struct sbuf *parent;
+    uint32_t roff, woff;
+};
+
+static void sbuf_tmp_pre_save(void *opaque)
+{
+    struct sbuf_tmp *tmp = opaque;
+    tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
+    tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
+}
+
+static int sbuf_tmp_post_load(void *opaque, int version)
 {
-    uint32_t off;
-
-    qemu_put_be32(f, sbuf->sb_cc);
-    qemu_put_be32(f, sbuf->sb_datalen);
-    off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
-    qemu_put_sbe32(f, off);
-    off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
-    qemu_put_sbe32(f, off);
-    qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
+    struct sbuf_tmp *tmp = opaque;
+    uint32_t requested_len = tmp->parent->sb_datalen;
+
+    /* Allocate the buffer space used by the field after the tmp */
+    sbreserve(tmp->parent, tmp->parent->sb_datalen);
+
+    if (tmp->parent->sb_datalen != requested_len) {
+        return -ENOMEM;
+    }
+    if (tmp->woff >= requested_len ||
+        tmp->roff >= requested_len) {
+        error_report("invalid sbuf offsets r/w=%u/%u len=%u",
+                     tmp->roff, tmp->woff, requested_len);
+        return -EINVAL;
+    }
+
+    tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
+    tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
+
+    return 0;
 }
 
+
+static const VMStateDescription vmstate_slirp_sbuf_tmp = {
+    .name = "slirp-sbuf-tmp",
+    .post_load = sbuf_tmp_post_load,
+    .pre_save  = sbuf_tmp_pre_save,
+    .version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(woff, struct sbuf_tmp),
+        VMSTATE_UINT32(roff, struct sbuf_tmp),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_slirp_sbuf = {
+    .name = "slirp-sbuf",
+    .version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(sb_cc, struct sbuf),
+        VMSTATE_UINT32(sb_datalen, struct sbuf),
+        VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, vmstate_slirp_sbuf_tmp),
+        VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, sb_datalen),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
 static void slirp_socket_save(QEMUFile *f, struct socket *so)
 {
     qemu_put_be32(f, so->so_urgc);
@@ -1225,8 +1278,9 @@ static void slirp_socket_save(QEMUFile *f, struct socket *so)
     qemu_put_byte(f, so->so_emu);
     qemu_put_byte(f, so->so_type);
     qemu_put_be32(f, so->so_state);
-    slirp_sbuf_save(f, &so->so_rcv);
-    slirp_sbuf_save(f, &so->so_snd);
+    /* 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);
 }
 
@@ -1263,31 +1317,9 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
     slirp_bootp_save(f, slirp);
 }
 
-static int slirp_sbuf_load(QEMUFile *f, struct sbuf *sbuf)
-{
-    uint32_t off, sb_cc, sb_datalen;
-
-    sb_cc = qemu_get_be32(f);
-    sb_datalen = qemu_get_be32(f);
-
-    sbreserve(sbuf, sb_datalen);
-
-    if (sbuf->sb_datalen != sb_datalen)
-        return -ENOMEM;
-
-    sbuf->sb_cc = sb_cc;
-
-    off = qemu_get_sbe32(f);
-    sbuf->sb_wptr = sbuf->sb_data + off;
-    off = qemu_get_sbe32(f);
-    sbuf->sb_rptr = sbuf->sb_data + off;
-    qemu_get_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
-
-    return 0;
-}
-
 static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
 {
+    int ret = 0;
     if (tcp_attach(so) < 0)
         return -ENOMEM;
 
@@ -1324,11 +1356,15 @@ static int slirp_socket_load(QEMUFile *f, struct socket *so, int version_id)
     so->so_emu = qemu_get_byte(f);
     so->so_type = qemu_get_byte(f);
     so->so_state = qemu_get_be32(f);
-    if (slirp_sbuf_load(f, &so->so_rcv) < 0)
-        return -ENOMEM;
-    if (slirp_sbuf_load(f, &so->so_snd) < 0)
-        return -ENOMEM;
-    return vmstate_load_state(f, &vmstate_slirp_tcp, so->so_tcpcb, 0);
+    /* 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)
-- 
2.9.3

[Qemu-devel] [PATCH v4 3/5] slirp: Common lhost/fhost union

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The socket structure has a pair of unions for lhost and fhost
addresses; the unions are identical so split them out into
a separate union declaration.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 slirp/socket.h | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/slirp/socket.h b/slirp/socket.h
index 8feed2a..c1be77e 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -15,6 +15,12 @@
  * Our socket structure
  */
 
+union slirp_sockaddr {
+    struct sockaddr_storage ss;
+    struct sockaddr_in sin;
+    struct sockaddr_in6 sin6;
+};
+
 struct socket {
   struct socket *so_next,*so_prev;      /* For a linked list of sockets */
 
@@ -31,22 +37,14 @@ struct socket {
   struct tcpiphdr *so_ti;	   /* Pointer to the original ti within
 				    * so_mconn, for non-blocking connections */
   int so_urgc;
-  union {   /* foreign host */
-      struct sockaddr_storage ss;
-      struct sockaddr_in sin;
-      struct sockaddr_in6 sin6;
-  } fhost;
+  union slirp_sockaddr fhost;      /* Foreign host */
 #define so_faddr fhost.sin.sin_addr
 #define so_fport fhost.sin.sin_port
 #define so_faddr6 fhost.sin6.sin6_addr
 #define so_fport6 fhost.sin6.sin6_port
 #define so_ffamily fhost.ss.ss_family
 
-  union {   /* local host */
-      struct sockaddr_storage ss;
-      struct sockaddr_in sin;
-      struct sockaddr_in6 sin6;
-  } lhost;
+  union slirp_sockaddr lhost;      /* Local host */
 #define so_laddr lhost.sin.sin_addr
 #define so_lport lhost.sin.sin_port
 #define so_laddr6 lhost.sin6.sin6_addr
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 3/5] slirp: Common lhost/fhost union

[Philippe Mathieu-Daudé]

On 02/20/2017 03:50 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The socket structure has a pair of unions for lhost and fhost
> addresses; the unions are identical so split them out into
> a separate union declaration.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  slirp/socket.h | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/slirp/socket.h b/slirp/socket.h
> index 8feed2a..c1be77e 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -15,6 +15,12 @@
>   * Our socket structure
>   */
>
> +union slirp_sockaddr {
> +    struct sockaddr_storage ss;
> +    struct sockaddr_in sin;
> +    struct sockaddr_in6 sin6;
> +};
> +
>  struct socket {
>    struct socket *so_next,*so_prev;      /* For a linked list of sockets */
>
> @@ -31,22 +37,14 @@ struct socket {
>    struct tcpiphdr *so_ti;	   /* Pointer to the original ti within
>  				    * so_mconn, for non-blocking connections */
>    int so_urgc;
> -  union {   /* foreign host */
> -      struct sockaddr_storage ss;
> -      struct sockaddr_in sin;
> -      struct sockaddr_in6 sin6;
> -  } fhost;
> +  union slirp_sockaddr fhost;      /* Foreign host */
>  #define so_faddr fhost.sin.sin_addr
>  #define so_fport fhost.sin.sin_port
>  #define so_faddr6 fhost.sin6.sin6_addr
>  #define so_fport6 fhost.sin6.sin6_port
>  #define so_ffamily fhost.ss.ss_family
>
> -  union {   /* local host */
> -      struct sockaddr_storage ss;
> -      struct sockaddr_in sin;
> -      struct sockaddr_in6 sin6;
> -  } lhost;
> +  union slirp_sockaddr lhost;      /* Local host */
>  #define so_laddr lhost.sin.sin_addr
>  #define so_lport lhost.sin.sin_port
>  #define so_laddr6 lhost.sin6.sin6_addr
>

Re: [Qemu-devel] [PATCH v4 3/5] slirp: Common lhost/fhost union

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The socket structure has a pair of unions for lhost and fhost
> addresses; the unions are identical so split them out into
> a separate union declaration.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

[Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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

A place holder for IPv6 support is added as a comment; it needs
testing once the rest of the IPv6 code is there.

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

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 178c2b6..6583de8 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1250,40 +1250,99 @@ 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;
 }
 
+#ifndef _WIN32
+#define VMSTATE_SS_FAMILY(f, s) VMSTATE_UINT16(f, s)
+#define VMSTATE_SIN4_ADDR(f, s, t) VMSTATE_UINT32_TEST(f, s, t)
+#else
+/* Win has a signed family number */
+#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
+/* Win uses u_long rather than uint32_t - but it's still 32bits long */
+#define VMSTATE_SIN4_ADDR(f, s, t) VMSTATE_SINGLE_TEST(f, s, t, 0, \
+                                       vmstate_info_uint32, u_long)
+#endif
+
+static const VMStateDescription vmstate_slirp_socket_addr = {
+    .name = "slirp-socket-addr",
+    .version_id = 4,
+    .fields = (VMStateField[]) {
+        VMSTATE_SS_FAMILY(ss.ss_family, union slirp_sockaddr),
+        VMSTATE_SIN4_ADDR(sin.sin_addr.s_addr, union slirp_sockaddr,
+                            slirp_family_inet),
+        VMSTATE_UINT16_TEST(sin.sin_port, union slirp_sockaddr,
+                            slirp_family_inet),
+
+#if 0
+        /* Untested: Needs checking by someone with IPv6 test */
+        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),
+#endif
+
+        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_SIN4_ADDR(so_faddr.s_addr, struct socket,
+                            slirp_older_than_v4),
+        VMSTATE_SIN4_ADDR(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 +1367,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 +1376,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 +1399,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 */
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Juan Quintela]

"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.
>
> A place holder for IPv6 support is added as a comment; it needs
> testing once the rest of the IPv6 code is there.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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


> +/* Win has a signed family number */
> +#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)

Great!
Just hope that there is no 32000 families soon :-)


> +#if 0
> +        /* Untested: Needs checking by someone with IPv6 test */
> +        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),
> +#endif

I think this is wrong, we have different fieldbs depending in if it is
ipv6 or not?

Later, Juan.

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "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.
> >
> > A place holder for IPv6 support is added as a comment; it needs
> > testing once the rest of the IPv6 code is there.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
> > +/* Win has a signed family number */
> > +#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
> 
> Great!
> Just hope that there is no 32000 families soon :-)
> 
> 
> > +#if 0
> > +        /* Untested: Needs checking by someone with IPv6 test */
> > +        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),
> > +#endif
> 
> I think this is wrong, we have different fieldbs depending in if it is
> ipv6 or not?

Yes, if the family field tells us it's IPv6 then we have to send
the longer and the extra IPv6 info - I don't know IPv6, I just
used the fields in the sin6 branch of the address and left
it in there at Samuel's suggestion for the person who
actually implements IPv6 support in this part of SLIRP.

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

Samual, Jan: Can you just take 1-3 of this series;  4 has a problem;

* Juan Quintela (quintela@redhat.com) wrote:
> "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.
> >
> > A place holder for IPv6 support is added as a comment; it needs
> > testing once the rest of the IPv6 code is there.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
> > +/* Win has a signed family number */
> > +#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
> 
> Great!
> Just hope that there is no 32000 families soon :-)

Actually; that's a problem;  it turns out FreeBSD has a char for it's
family type rahter than the uint16 that linux and windows have.
I need to think hth to abstract that.

Dave


> 
> 
> > +#if 0
> > +        /* Untested: Needs checking by someone with IPv6 test */
> > +        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),
> > +#endif
> 
> I think this is wrong, we have different fieldbs depending in if it is
> ipv6 or not?
> 
> Later, Juan.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Daniel P. Berrange]

On Thu, Feb 23, 2017 at 10:51:56AM +0000, Dr. David Alan Gilbert wrote:
> Samual, Jan: Can you just take 1-3 of this series;  4 has a problem;
> 
> * Juan Quintela (quintela@redhat.com) wrote:
> > "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.
> > >
> > > A place holder for IPv6 support is added as a comment; it needs
> > > testing once the rest of the IPv6 code is there.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > 
> > 
> > > +/* Win has a signed family number */
> > > +#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
> > 
> > Great!
> > Just hope that there is no 32000 families soon :-)
> 
> Actually; that's a problem;  it turns out FreeBSD has a char for it's
> family type rahter than the uint16 that linux and windows have.
> I need to think hth to abstract that.

What, if any, promises have we made about the migration data format
being ABI stable across platforms ?

It strikes me that even if FreeBSD & Linux had the same sized type
for ss_family, the constants used to popuate it might be different
on each platform. eg AF_INET6 is 10 on Linux but 23 on Mingw and
probably something else again on FreeBSD.

IOW if we transmit this data on the wire, we've effectively said that
our migration data format is *not* portable across different host OS
platforms. At that point, sending different sized types on BSD vs
Linux is no big deal since we're already incompatible semantically.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Thu, Feb 23, 2017 at 10:51:56AM +0000, Dr. David Alan Gilbert wrote:
> > Samual, Jan: Can you just take 1-3 of this series;  4 has a problem;
> > 
> > * Juan Quintela (quintela@redhat.com) wrote:
> > > "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.
> > > >
> > > > A place holder for IPv6 support is added as a comment; it needs
> > > > testing once the rest of the IPv6 code is there.
> > > >
> > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > 
> > > 
> > > > +/* Win has a signed family number */
> > > > +#define VMSTATE_SS_FAMILY(f, s) VMSTATE_INT16(f, s)
> > > 
> > > Great!
> > > Just hope that there is no 32000 families soon :-)
> > 
> > Actually; that's a problem;  it turns out FreeBSD has a char for it's
> > family type rahter than the uint16 that linux and windows have.
> > I need to think hth to abstract that.
> 
> What, if any, promises have we made about the migration data format
> being ABI stable across platforms ?

It should be in theory; it should just reflect the state of the guest, and
the state of the guest should be independent of the platform it's running on.
Now slirp is a bit of an odd case since it involves more host state.

> It strikes me that even if FreeBSD & Linux had the same sized type
> for ss_family, the constants used to popuate it might be different
> on each platform. eg AF_INET6 is 10 on Linux but 23 on Mingw and
> probably something else again on FreeBSD.

Ewww I'd thought that was at least standardised.
It looks like AF_INET is 2 on Linux, mingw and FreeBSD;
but AF_INET6 is more dependent on what snook in before hand.

> IOW if we transmit this data on the wire, we've effectively said that
> our migration data format is *not* portable across different host OS
> platforms. At that point, sending different sized types on BSD vs
> Linux is no big deal since we're already incompatible semantically.

This is a relatively recent error; it comes from eae303ff which added
ss_family to allow IPv6.

I suspect the right fix here is to populate a temporary for ss_family
on the wire; if we make it conveniently match Linux's AF_INET6 values
it might work.

Dave

> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Hello,

Dr. David Alan Gilbert, on jeu. 23 févr. 2017 11:40:45 +0000, wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > IOW if we transmit this data on the wire, we've effectively said that
> > our migration data format is *not* portable across different host OS
> > platforms. At that point, sending different sized types on BSD vs
> > Linux is no big deal since we're already incompatible semantically.
> 
> This is a relatively recent error; it comes from eae303ff which added
> ss_family to allow IPv6.
> 
> I suspect the right fix here is to populate a temporary for ss_family
> on the wire; if we make it conveniently match Linux's AF_INET6 values
> it might work.

So this issue is actually not new, so I asked to pull your patch series,
thanks!

Now we should indeed fix the issue. Making the values match Linux'
AF_INET6, why not (better have some known values than arbitrary values),
but I don't think we'd want to rely on this: we'll have to introduce a
versioned difference, since we'll want to change the size of the field
as well as its value on other OSes (and I don't think we want to manage
different versioning on different OSes).

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Samuel Thibault, on dim. 26 févr. 2017 21:34:27 +0100, wrote:
> since we'll want to change the size of the field

Ah, no, sorry, it was forced to be 16bit, so at least the size is fine.

But I guess we don't want to change the values to have cross-OS
compatibility without changing the version.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Hello,
> 
> Dr. David Alan Gilbert, on jeu. 23 févr. 2017 11:40:45 +0000, wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > IOW if we transmit this data on the wire, we've effectively said that
> > > our migration data format is *not* portable across different host OS
> > > platforms. At that point, sending different sized types on BSD vs
> > > Linux is no big deal since we're already incompatible semantically.
> > 
> > This is a relatively recent error; it comes from eae303ff which added
> > ss_family to allow IPv6.
> > 
> > I suspect the right fix here is to populate a temporary for ss_family
> > on the wire; if we make it conveniently match Linux's AF_INET6 values
> > it might work.
> 
> So this issue is actually not new, so I asked to pull your patch series,
> thanks!

It is because the old non-vmstate code just truncated the value read,
where as the VMSTATE macros have strong type checks.

> Now we should indeed fix the issue. Making the values match Linux'
> AF_INET6, why not (better have some known values than arbitrary values),
> but I don't think we'd want to rely on this: we'll have to introduce a
> versioned difference, since we'll want to change the size of the field
> as well as its value on other OSes (and I don't think we want to manage
> different versioning on different OSes).

> Ah, no, sorry, it was forced to be 16bit, so at least the size is fine.
> 
> But I guess we don't want to change the values to have cross-OS
> compatibility without changing the version.

I'm thinking one way to do it without changing the version would
be to use the existing value for IPv4, and on reading allow any other
value for IPv6 (or just the ones we know about); that would make
it inwards migration compatible.  For writing we pick one for IPv6
and stick to it.

Dave

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

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +0000, wrote:
> I'm thinking one way to do it without changing the version would
> be to use the existing value for IPv4, and on reading allow any other
> value for IPv6 (or just the ones we know about); that would make
> it inwards migration compatible.

Right. I don't know if that's enough for QEMU requirements.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +0000, wrote:
> > I'm thinking one way to do it without changing the version would
> > be to use the existing value for IPv4, and on reading allow any other
> > value for IPv6 (or just the ones we know about); that would make
> > it inwards migration compatible.
> 
> Right. I don't know if that's enough for QEMU requirements.

If you change the version number you break backwards migration anyway;
but doing what I suggested would keep backwards working in most cases.

Dave

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

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:00:17 +0000, wrote:
> * Samuel Thibault (samuel.thibault@gnu.org) wrote:
> > Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +0000, wrote:
> > > I'm thinking one way to do it without changing the version would
> > > be to use the existing value for IPv4, and on reading allow any other
> > > value for IPv6 (or just the ones we know about); that would make
> > > it inwards migration compatible.
> > 
> > Right. I don't know if that's enough for QEMU requirements.
> 
> If you change the version number you break backwards migration anyway;
> but doing what I suggested would keep backwards working in most cases.

Sure, but in some cases we're breaking upward compatibility *silently*.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:00:17 +0000, wrote:
> > * Samuel Thibault (samuel.thibault@gnu.org) wrote:
> > > Dr. David Alan Gilbert, on mar. 28 févr. 2017 16:54:46 +0000, wrote:
> > > > I'm thinking one way to do it without changing the version would
> > > > be to use the existing value for IPv4, and on reading allow any other
> > > > value for IPv6 (or just the ones we know about); that would make
> > > > it inwards migration compatible.
> > > 
> > > Right. I don't know if that's enough for QEMU requirements.
> > 
> > If you change the version number you break backwards migration anyway;
> > but doing what I suggested would keep backwards working in most cases.
> 
> Sure, but in some cases we're breaking upward compatibility *silently*.

No, we don't need to break upwards compatibility; if I understand correctly
the only values that can be in that field at the moment are:
  AF_INET - IPv4 which is 2 on everything we've seen
  AF_INET6 - whose value is different on different implementations.

If we accept an incoming value of '2' to mean AF_INET then all existing
IPv4 connections work.
If we accept any other incoming value as AF_INET6 all existing IPv6 connections
work.
   - so that's foward compatibility OK.

If we generate '2' as the outgoing value for AF_INET then migration
to all exiting code works for IPv4; so backwards works for IPv4.
If we generate Linux's AF_INET6 number then IPv6 also works backwards
but only for Linux.

Dave

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

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:09:26 +0000, wrote:
> but only for Linux.

That's what I was referring to.

Samuel

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Dr. David Alan Gilbert]

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:09:26 +0000, wrote:
> > but only for Linux.
> 
> That's what I was referring to.

Yes I think that's OK because:
   a) The alternative breaks all backwards migration
   b) This doesn't casuse any problem for forward migration

There is another way, we could add a subsection that's sent whenever
we use non-IPv4; that would (probably) fail noisily on a backwards
migration to an old setup.  But IMHO it's not worth it in this case
given how rare a backwards migration of an IPv6 slirp connection on
something other than Linux is.

Dave

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

Re: [Qemu-devel] [PATCH v4 4/5] slirp: VMStatify socket level

[Samuel Thibault]

Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:40:19 +0000, wrote:
> * Samuel Thibault (samuel.thibault@gnu.org) wrote:
> > Dr. David Alan Gilbert, on mar. 28 févr. 2017 17:09:26 +0000, wrote:
> > > but only for Linux.
> > 
> > That's what I was referring to.
> 
> Yes I think that's OK because:
>    a) The alternative breaks all backwards migration
>    b) This doesn't casuse any problem for forward migration
> 
> There is another way, we could add a subsection that's sent whenever
> we use non-IPv4; that would (probably) fail noisily on a backwards
> migration to an old setup.  But IMHO it's not worth it in this case
> given how rare a backwards migration of an IPv6 slirp connection on
> something other than Linux is.

Alright.

Samuel

[Qemu-devel] [PATCH v4 5/5] slirp: VMStatify remaining except for loop

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This converts the remaining components, except for the top level
loop, to VMState.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 slirp/slirp.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/slirp/slirp.c b/slirp/slirp.c
index 6583de8..f8ee7f9 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -1343,15 +1343,25 @@ static const VMStateDescription vmstate_slirp_socket = {
     }
 };
 
-static void slirp_bootp_save(QEMUFile *f, Slirp *slirp)
-{
-    int i;
+static const VMStateDescription vmstate_slirp_bootp_client = {
+    .name = "slirp_bootpclient",
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(allocated, BOOTPClient),
+        VMSTATE_BUFFER(macaddr, BOOTPClient),
+        VMSTATE_END_OF_LIST()
+    }
+};
 
-    for (i = 0; i < NB_BOOTP_CLIENTS; i++) {
-        qemu_put_be16(f, slirp->bootp_clients[i].allocated);
-        qemu_put_buffer(f, slirp->bootp_clients[i].macaddr, 6);
+static const VMStateDescription vmstate_slirp = {
+    .name = "slirp",
+    .version_id = 4,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16_V(ip_id, Slirp, 2),
+        VMSTATE_STRUCT_ARRAY(bootp_clients, Slirp, NB_BOOTP_CLIENTS, 3,
+                             vmstate_slirp_bootp_client, BOOTPClient),
+        VMSTATE_END_OF_LIST()
     }
-}
+};
 
 static void slirp_state_save(QEMUFile *f, void *opaque)
 {
@@ -1371,22 +1381,10 @@ static void slirp_state_save(QEMUFile *f, void *opaque)
         }
     qemu_put_byte(f, 0);
 
-    qemu_put_be16(f, slirp->ip_id);
-
-    slirp_bootp_save(f, slirp);
+    vmstate_save_state(f, &vmstate_slirp, slirp, NULL);
 }
 
 
-static void slirp_bootp_load(QEMUFile *f, Slirp *slirp)
-{
-    int i;
-
-    for (i = 0; i < NB_BOOTP_CLIENTS; i++) {
-        slirp->bootp_clients[i].allocated = qemu_get_be16(f);
-        qemu_get_buffer(f, slirp->bootp_clients[i].macaddr, 6);
-    }
-}
-
 static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
 {
     Slirp *slirp = opaque;
@@ -1421,13 +1419,5 @@ static int slirp_state_load(QEMUFile *f, void *opaque, int version_id)
         so->extra = (void *)ex_ptr->ex_exec;
     }
 
-    if (version_id >= 2) {
-        slirp->ip_id = qemu_get_be16(f);
-    }
-
-    if (version_id >= 3) {
-        slirp_bootp_load(f, slirp);
-    }
-
-    return 0;
+    return vmstate_load_state(f, &vmstate_slirp, slirp, version_id);
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 5/5] slirp: VMStatify remaining except for loop

[Juan Quintela]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> This converts the remaining components, except for the top level
> loop, to VMState.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

Re: [Qemu-devel] [PATCH v4 0/5] SLIRP VMStatification

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH v4 0/5] SLIRP VMStatification
Message-id: 20170220185020.774-1-dgilbert@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1487597638-28323-1-git-send-email-armbru@redhat.com -> patchew/1487597638-28323-1-git-send-email-armbru@redhat.com
 * [new tag]         patchew/20170220185020.774-1-dgilbert@redhat.com -> patchew/20170220185020.774-1-dgilbert@redhat.com
Switched to a new branch 'test'
42ec6e4 slirp: VMStatify remaining except for loop
8278228 slirp: VMStatify socket level
b2cd17c slirp: Common lhost/fhost union
af8192f slirp: VMStatify sbuf
3bb572c slirp: VMState conversion; tcpcb

=== OUTPUT BEGIN ===
Checking PATCH 1/5: slirp: VMState conversion; tcpcb...
ERROR: code indent should never use tabs
#211: FILE: slirp/tcp_var.h:51:
+^Iuint8_t t_force;^I^I/* 1 if forcing out a byte */$

ERROR: code indent should never use tabs
#221: FILE: slirp/tcp_var.h:112:
+^Iuint8_t^It_oobflags;^I^I/* have some */$

ERROR: code indent should never use tabs
#222: FILE: slirp/tcp_var.h:113:
+^Iuint8_t^It_iobc;^I^I^I/* input character */$

total: 3 errors, 0 warnings, 195 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/5: slirp: VMStatify sbuf...
ERROR: code indent should never use tabs
#26: FILE: slirp/sbuf.h:15:
+^Iuint32_t sb_cc;^I^I/* actual chars in buffer */$

ERROR: code indent should never use tabs
#27: FILE: slirp/sbuf.h:16:
+^Iuint32_t sb_datalen;^I/* Length of data  */$

total: 2 errors, 0 warnings, 155 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/5: slirp: Common lhost/fhost union...
Checking PATCH 4/5: slirp: VMStatify socket level...
ERROR: if this code is redundant consider removing it
#96: FILE: slirp/slirp.c:1297:
+#if 0

total: 1 errors, 0 warnings, 217 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/5: slirp: VMStatify remaining except for loop...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org