Re: [Xen-devel] [PATCH 2/2] libxl: save/restore qemu's physmap

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 20 Jan 2012, Ian Campbell wrote:
> On Fri, 2012-01-20 at 11:18 +0000, Stefano Stabellini wrote:
> > Read Qemu's physmap from xenstore and save it using toolstack_save.
> > Restore Qemu's physmap using toolstack_restore.
> 
> Shall we have a version field now so we don't dig ourselves a hole we
> can't get out of?

good idea

> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_dom.c |  132 
> > ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 131 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index fd2b051..3d60a35 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -347,6 +347,62 @@ out:
> >      return rc;
> >  }
> >  
> > +static int libxl__toolstack_restore(uint32_t domid, uint8_t *buf,
> > +        uint32_t size, void *data)
> > +{
> > +    libxl__gc *gc = (libxl__gc *) data;
> > +    int i, ret;
> > +    uint8_t *ptr = buf;
> > +    uint32_t namelen = 0;
> > +    char *name = NULL;
> > +    uint32_t count = 0;
> > +    uint64_t phys_offset_v = 0, start_addr_v = 0, size_v = 0;
> > +
> > +    if (size < sizeof(count))
> > +        return -1;
> > +
> > +    memcpy(&count, ptr, sizeof(count));
> > +    ptr += sizeof(count);
> > + 
> > +    if (size <
> > +            sizeof(count) + count * (sizeof(uint64_t) * 3 + 
> > sizeof(uint32_t)))
> > +        return -1;
> > +
> > +    for (i = 0; i < count; i++) {
> > +        memcpy(&namelen, ptr, sizeof(namelen));
> > +        ptr += sizeof(namelen);
> > +        if (namelen > 0) {
> > +            name = (char *)ptr;
> > +            ptr += namelen;
> > +        }
> > +        memcpy(&phys_offset_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> > +        memcpy(&start_addr_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> > +        memcpy(&size_v, ptr, sizeof(uint64_t));
> > +        ptr += sizeof(uint64_t);
> 
> Why not define a struct libxl__physmap_info for these three? You could
> even do the trick with the "char name[]" at the end and incorporate
> namelen too if you wanted.
> 
> The maximum size of the allocation is
>       sizeof(count) + count * (sizeof(uint64_t) * 3 + sizeof(uint32_t)))
> here but in the save it is allocating:
>       sizeof(count) + count * (sizeof(val) * 3 + sizeof(namelen));
> these work out the same but it would be more obviously correct if it
> were "count * sizeof(struct)"
> 
> Actually, hang on where is the space for name itself allocated? I see,
> you are reallocing. If you have to do that anyway you may as well start
> off with just sizeof(count) and realloc namelen + sizeof(struct) at each
> stage.

I played with the idea of the struct for a bit and it has improved the
code, especially on restore.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel