Re: [BUGFIX][PATCH 3/4] hvm_save_one: return correct data.

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3263 bytes --]

On 16/12/2013 17:51, Don Slutz wrote:
> On 12/16/13 03:17, Jan Beulich wrote:
>>>>> On 15.12.13 at 17:51, Andrew Cooper <andrew.cooper3@citrix.com> 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.
>> Oh, yes, indeed.
>>
>>> 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 solution I can see is that for the per-vcpu records, the save
>>> functions get refactored to take an instance ID, and only save their
>>> specific instance.
>> I don't see why you shouldn't be able to look at the descriptor
>> instead - that one does have the correct size, doesn't it?
>>
>> Jan
>>
> Attached is v3 of this.  It is basically a merge of patch #3 and patch
> #4 with cleanups.
>
> This is what I said in:
>
> http://lists.xen.org/archives/html/xen-devel/2013-12/msg02216.html
>
> and Andrew replied in:
>
> http://lists.xen.org/archives/html/xen-devel/2013-12/msg02217.html
>
> and the RFC is:
>
> http://lists.xen.org/archives/html/xen-devel/2013-12/msg02223.html
>
> to which:
>
> http://lists.xen.org/archives/html/xen-devel/2013-12/msg02270.html
>
> (from this):
>
>         IMHO this is obviously not 4.4 material at this stage. Apart from
>         anything else we've been managing to release with these short comings
>         for many years.
>
>     Indeed. -George
>
> I feel that the attached bugfix patch is simple enough to make it into 4.4 and also be back ported to stable branches.
>
>    -Don Slutz
>
>
>
>
>

Your loop condition needs to change be "off < (ctxt.cur -
sizeof(*desc))" otherwise the "off +=  sizeof(*desc)" can wander beyond
ctxt.cur in the loop body.  You also need to verify that the
copy_to_guest doesn't exceed ctxt.cur.

Stylistically, "desc = (void *)ctxt.data + off;" needs to be "desc =
(void *)(ctxt.data + off);" as the latter is standards compliment C
while the former is UB which GCC has an extension to deal with sensibly.

Also you have a double space before sizeof in "off +=  sizeof(*desc);"

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5544 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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