* Halil Pasic (pa...@linux.vnet.ibm.com) wrote: > > > On 10/17/2016 05:36 AM, David Gibson wrote: > > On Tue, Oct 11, 2016 at 06:18:32PM +0100, Dr. David Alan Gilbert (git) > > wrote: > >> From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@redhat.com> > > Hi Dave! > > I had a brief look, which means I intend to have a deeper > one too, but for now you will have to live with this.
Thanks. > > > > Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> > > > >> --- > >> 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 6276315..2f7802e 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) > >> { > > What makes me think about the properties of this approach, > is, that each time we use a parent pointer to read we have > a data dependency. This seems to me much more complicated > that the current massaging function approach were we say > "OK now everything below me is there, now let us transform". > Of course the proposed approach is more powerful. Yes it is, but we have to apply a transform to the data so that means we somehow need to get to both a temporary piece of storage and the parent data. > >> - 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; > > Ok, data parent->sb_datalen was previously loaded at #1 > > >> + > >> + /* Allocate the buffer space used by the field after the tmp */ > >> + sbreserve(tmp->parent, tmp->parent->sb_datalen); > #2 > >> + > >> + 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; > > Ok, parent->sb_data is assigned and the backing memory allocated > at #2 > > >> + > >> + 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), > > #1 > > >> + VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp, > >> vmstate_slirp_sbuf_tmp), > >> + VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0, > >> sb_datalen), > > OK, memory was allocated at #2 > It is a bit confusing though (for a novice like me) that we have a non ALLOC > VBUFFER > whose pointer is NULL after post_load. I don't think this pointer can be NULL; the sbreserve at #2 causes it to be allocated. But yes, it's a shame I can't use VMS_ALLOC here, but the sbreserve is not a trivial allocation function. > Now if I imagine the original stream were written in the following sequence: > vbuffer_length (sb_datalen), vbuffer_data (sb_data), offsets (sb_wptr, > sb_rptr) > which seems completely valid to me then the context would not be sufficient > to compute sb_wptr and sb_rptr because the lifetime of vbuffer_data and > the tmp do not overlap. If that was the case you could still do it pretty easily. You'd have to add the sb_datalen and sb_data fields to the temporary and then move the VMSTATE_VBUFFER_UINT32 into the tmp so it would operate on the copied fields. > I aware it's a trade-off between how long the temporary data lives and > how complicated the dependencies get. Or am I getting something wrong? No, I think that's right. The other option I thought of was a macro to allocate a temporary and then another to free it and then someway to tell macros in between that they should operate on the temporary rather than the main pointer; but then you'd have to be VERY careful to not allow yourself to access a temporary that's been freed. This structure means you can't make that mistake. Dave > > Cheers, > Halil > > >> + VMSTATE_END_OF_LIST() > >> + } > >> +}; > [..] > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK