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

[Don Slutz <dslutz@verizon.com>]

[-- Attachment #1: Type: text/plain, Size: 4399 bytes --]

On 12/13/13 09:20, Jan Beulich wrote:
>>>> On 12.12.13 at  01:56, Don Slutz <dslutz@verizon.com> wrote:
 >> From: Don Slutz <dslutz@verizon.com>
 >>
 >> 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 needs to be cleaned up coding style wise and ...
 >
 >> --- a/xen/common/hvm/save.c +++ b/xen/common/hvm/save.c @@ -112,13
 >> +112,27 @@ int hvm_save_one(struct domain *d, uint16_t typecode,
 >> uint16_t instance, d->domain_id, typecode); rv = -EFAULT; } -
 >> else if ( copy_to_guest(handle, -
 >> ctxt.data -                            + (instance *
 >> hvm_sr_handlers[typecode].size) -                            +
 >> sizeof (struct hvm_save_descriptor), -
 >> hvm_sr_handlers[typecode].size -                            -
 >> sizeof (struct hvm_save_descriptor)) ) -        rv = -EFAULT; +
 >> else +    { +        uint32_t off; + +        rv = -EBADSLT; +
 >> for (off = 0; off < ctxt.cur; off +=
 >> hvm_sr_handlers[typecode].size) { +            struct
 >> hvm_save_descriptor *desc +                   = (struct
 >> hvm_save_descriptor *)&ctxt.data[off];
 >
 > .. this could be const, and the cast could simply be (void *), ...
 >
 >> +            if (instance == desc->instance) { +                rv
 >> = 0; +                if ( copy_to_guest(handle, +
 >> ctxt.data +                                   + off
 >
 > ... this doesn't need to be on a separate line, and ...
 >
 >> +                                   + sizeof (struct
 >> hvm_save_descriptor), +
 >> hvm_sr_handlers[typecode].size +
 >> - sizeof (struct hvm_save_descriptor)) )
 >
 > ... both these sizeof()s would now better be sizeof(*desc).
 >
 > Jan
 >
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>
---
  xen/common/hvm/save.c | 28 +++++++++++++++++++++-------
  1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index de76ada..6aaea6f 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -112,13 +112,27 @@ int hvm_save_one(struct domain *d, uint16_t 
typecode, uint16_t instance,
                 d->domain_id, typecode);
          rv = -EFAULT;
      }
-    else if ( copy_to_guest(handle,
-                            ctxt.data
-                            + (instance * hvm_sr_handlers[typecode].size)
-                            + sizeof (struct hvm_save_descriptor),
-                            hvm_sr_handlers[typecode].size
-                            - sizeof (struct hvm_save_descriptor)) )
-        rv = -EFAULT;
+    else
+    {
+        uint32_t off;
+
+        rv = -EBADSLT;
+        for ( off = 0; off < ctxt.cur; off += 
hvm_sr_handlers[typecode].size )
+        {
+            const struct hvm_save_descriptor *desc = (void 
*)&ctxt.data[off];
+
+            if ( instance == desc->instance )
+            {
+                rv = 0;
+                if ( copy_to_guest(handle,
+                                   ctxt.data + off + sizeof(*desc),
+                                   hvm_sr_handlers[typecode].size
+                                   - sizeof(*desc)) )
+                    rv = -EFAULT;
+                break;
+            }
+        }
+    }

      xfree(ctxt.data);
      return rv;
-- 
1.7.11.7

    -Don Slutz


[-- Attachment #2: 0003-hvm_save_one-return-correct-data.patch --]
[-- Type: text/x-patch, Size: 2044 bytes --]

>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>
---
 xen/common/hvm/save.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index de76ada..6aaea6f 100644
--- a/xen/common/hvm/save.c
+++ b/xen/common/hvm/save.c
@@ -112,13 +112,27 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance,
                d->domain_id, typecode);
         rv = -EFAULT;
     }
-    else if ( copy_to_guest(handle,
-                            ctxt.data 
-                            + (instance * hvm_sr_handlers[typecode].size) 
-                            + sizeof (struct hvm_save_descriptor), 
-                            hvm_sr_handlers[typecode].size
-                            - sizeof (struct hvm_save_descriptor)) )
-        rv = -EFAULT;
+    else
+    {
+        uint32_t off;
+
+        rv = -EBADSLT;
+        for ( off = 0; off < ctxt.cur; off += hvm_sr_handlers[typecode].size )
+        {
+            const struct hvm_save_descriptor *desc = (void *)&ctxt.data[off];
+
+            if ( instance == desc->instance )
+            {
+                rv = 0;
+                if ( copy_to_guest(handle,
+                                   ctxt.data + off + sizeof(*desc),
+                                   hvm_sr_handlers[typecode].size
+                                   - sizeof(*desc)) )
+                    rv = -EFAULT;
+                break;
+            }
+        }
+    }
 
     xfree(ctxt.data);
     return rv;
-- 
1.7.11.7


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

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