Re: [Xen-devel] [PATCH 2/2] HVM: clean up hvm_save_one()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.17 at 19:52, <wei.liu2@xxxxxxxxxx> wrote:
> On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
>> Eliminate the for_each_vcpu() loop and the associated local variables,
>> don't override the save handler's return code, and correct formatting.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> --- a/xen/common/hvm/save.c
>> +++ b/xen/common/hvm/save.c
>> @@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
>>  int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int 
> instance,
>>                   XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
>>  {
>> -    int rv = -ENOENT;
>> -    size_t sz = 0;
>> -    struct vcpu *v;
>> -    hvm_domain_context_t ctxt = { 0, };
>> +    int rv;
>> +    hvm_domain_context_t ctxt = { };
>>      const struct hvm_save_descriptor *desc;
>>  
>> -    if ( d->is_dying 
>> -         || typecode > HVM_SAVE_CODE_MAX 
>> -         || hvm_sr_handlers[typecode].size < sizeof(*desc)
>> -         || hvm_sr_handlers[typecode].save == NULL )
>> +    if ( d->is_dying ||
>> +         typecode > HVM_SAVE_CODE_MAX ||
>> +         hvm_sr_handlers[typecode].size < sizeof(*desc) ||
>> +         !hvm_sr_handlers[typecode].save )
>>          return -EINVAL;
>>  
>> +    ctxt.size = hvm_sr_handlers[typecode].size;
>>      if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
>> -        for_each_vcpu(d, v)
>> -            sz += hvm_sr_handlers[typecode].size;
>> -    else 
>> -        sz = hvm_sr_handlers[typecode].size;
>> -    
>> -    ctxt.size = sz;
>> -    ctxt.data = xmalloc_bytes(sz);
>> +        hvm_sr_handlers[typecode].size *= d->max_vcpus;
> 
> Why is size updated with a particular d->max_vcpus here? AFAICT (after
> going through layers of macros ...) hvm_sr_handlers is global and needed
> when saving any hvm guests. The "size" field contains the length of one
> record.
> 
> Also, you set ctxt.size before this loop without taking into account the
> number of vcpus, which looks wrong to me. Shouldn't it be (when not
> updating hvm_sr_handlers[typecode].size)
> 
>    ctxt.size = hvm_sr_handlers[typecode].size * d->max_vcpus
> 
> ?

Right, this is complete rubbish. Should be

ctxt.size *= d->max_vcpus;

>> +    ctxt.data = xmalloc_bytes(ctxt.size);
>>      if ( !ctxt.data )
>>          return -ENOMEM;
>>  
>> -    if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
>> -    {
>> -        printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
>> -               d->domain_id, typecode);
>> -        rv = -EFAULT;
>> -    }
>> -    else if ( ctxt.cur >= sizeof(*desc) )
>> +    if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
>> +        printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" 
>> (%d)\n",
>> +               d->domain_id, typecode, rv);
>> +    else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )
> 
> I guess the intent here is to set rv while at the same time only test
> ctxt.cur? But why?

Well, we can't use -ENOENT as initializer anymore, as rv now is
being modified above. Before entering the body of the "else if"
it needs to be -ENOENT though.

> Can the code be reorganised so that it is easier to reason about.

It probably could be, at the expense of assigning -ENOENT in two
places.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel