Re: [Qemu-devel] [Xen-devel] [XEN][RFC PATCH V2 11/17] xc: modify save/restore to support multiple device models

[Ian Campbell]

On Fri, 2012-08-24 at 11:27 +0100, Julien Grall wrote:
> On 08/23/2012 08:52 PM, Ian Campbell wrote:
> > On Thu, 2012-08-23 at 20:13 +0100, Julien Grall wrote:
> >    
> >> On 08/23/2012 02:27 PM, Ian Campbell wrote:
> >>      
> >>>        
> >>>> @@ -103,6 +103,9 @@ static ssize_t rdexact(xc_interface *xch, struct 
> >>>> restore_ctx *ctx,
> >>>>    #else
> >>>>    #define RDEXACT read_exact
> >>>>    #endif
> >>>> +
> >>>> +#define QEMUSIG_SIZE 21
> >>>> +
> >>>>    /*
> >>>>    ** In the state file (or during transfer), all page-table pages are
> >>>>    ** converted into a 'canonical' form where references to actual mfns
> >>>> @@ -467,7 +522,7 @@ static int buffer_tail_hvm(xc_interface *xch, struct 
> >>>> restore_ctx *ctx,
> >>>>                               int vcpuextstate, uint32_t 
> >>>> vcpuextstate_size)
> >>>>    {
> >>>>        uint8_t *tmp;
> >>>> -    unsigned char qemusig[21];
> >>>> +    unsigned char qemusig[QEMUSIG_SIZE + 1];
> >>>>
> >>>>          
> >>> An extra + 1 here?
> >>>
> >>>        
> >> QEMUSIG_SIZE doesn't take into account the '\0'. So we need to add 1.
> >> If an error occurred, without +1, the output log lost the last character.
> >>      
> > So this is just a bug fix for a pre-existing issue?
> >    
> Yes.

Can we get it as a separate change?

> 
> >>> [...]
> >>>
> >>>        
> >>>> -    qemusig[20] = '\0';
> >>>> +    qemusig[QEMUSIG_SIZE] = '\0';
> >>>>
> >>>>          
> >>> This is one bigger than it used to be now.
> >>>
> >>> Perhaps this is an unrelated bug fix (I haven't check the real length of
> >>> the sig), in which case please can you split it out and submit
> >>> separately?
> >>>
> >>>        
> >> #define QEMU_SIGNATURE "DeviceModelRecord0002"
> >> Just checked, the length seems to be 21. I will send a patch with
> >> this change.
> >>      
> > Perhaps use either sizeof(QEMU_SIGNATURE) or strlen(QEMU_SIGNATURE)
> > (depending on which semantics you want)?
> >    
> Here, QEMU_SIZE needs to be define as strlen (QEMU_SIGNATURE),
> but QEMU_SIGNATURE is not defined in libxc. It's defined
> in libxl/libxl_internal.h.

Oh, right, this again :-/

> By the way, I'm wondering why QEMU save (libxl__domain_save_device_model)
> is made in libxl and restore (dump_qemu) in libxc ?

Mostly historical accident, we'd really like to sort this out one way or
the other but untangling the protocol and the callbacks etc is a pretty
big job.

In the meantime perhaps libxc could provide a suitable "typedef char
device_model_signature_t[21]"?

Ian.