On 15/12/2013 17:19, Don Slutz wrote:
> On 12/15/13 11:51, Andrew Cooper wrote:
>> On 15/12/2013 00:29, Don Slutz wrote:
>>>
>>> I think I have corrected all coding errors (please check again). And
>>> done all requested changes.  I did add the reviewed by (not sure if
>>> I should since this changes a large part of the patch, but they are
>>> all what Jan said).
>>>
>>> I have unit tested it and it appears to work the same as the
>>> previous version (as expected).
>>>
>>> Here is the new version, also attached.
>>>
>>> From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001
>>> From: Don Slutz <dslutz@verizon.com>
>>> Date: Tue, 12 Nov 2013 08:22:53 -0500
>>> Subject: [PATCH v2 3/4] hvm_save_one: return correct data.
>>>
>>> It is possible that hvm_sr_handlers[typecode].save does not use all
>>> the provided room.  In that case, using:
>>>
>>>    instance * hvm_sr_handlers[typecode].size
>>>
>>> does not select the correct instance.  Add code to search for the
>>> correct instance.
>>>
>>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> but this fairs no better at selecting the correct subset in the case
>> that less data than hvm_sr_handlers[typecode].size is written by
>> hvm_sr_handlers[typecode].save.
>>
> True, but the inverse is the case here; .save writes 'n' 'size'
> blocks.  Form the loop above:
>
>     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;
>
> so sz is in multiples of 'size'.  Normally sz == ctxt.cur.  With some
> offline vcpus it write fewer 'size' blocks.
>> It always increments by 'size' bytes, and will only copy the data
>> back if the bytes under desc->instance happen to match the instance
>> we are looking for.
>>
> The only time it does not find one is for an offline vcpu.  Try out
> the unit test code in patch #1 on an unchanged xen.  It should not
> display anything.  Then offline a cpu in a domU (echo 0 >
> /sys/devices/system/cpu/cpu1/online).  And with 3 vcpus, it will
> report an error.
>
>    -Don Slutz

Ah - so there are actually two problems.  I see now the one you are
trying to solve, and would agree that your code does solve it.

However, some of the save handlers are themselves variable length, and
will write records shorter than hvm_sr_handlers[typecode].size if they
can get away with doing so.  In this case, the new logic still wont get
the correct instance.

~Andrew