[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 11/16] Add live migration of VMware's hyper-call RPC




On 09/12/14 09:54, Boris Ostrovsky wrote:
On 09/11/2014 02:36 PM, Don Slutz wrote:
The VMware's hyper-call state is included in live migration and
save/restore.  Because the max size of the VMware guestinfo is
large, then data is compressed and expanded in the
vmport_save_domain_ctxt and vmport_load_domain_ctxt.

Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
---



+
+    for ( i = 0; i < ctxt->used_guestinfo; i++ )
+    {
+        vmport_guestinfo_t *vg = vs->guestinfo[i];
+
+        if ( vg && vg->key_len )
+        {
+ guestinfo_size += sizeof(vg->key_len) + sizeof(vg->val_len) +
+                vg->key_len + vg->val_len;
+            used_guestinfo++;
+            ASSERT(sizeof(vg->key_len) == 1);
+            *p++ = (char) vg->key_len;
+            ASSERT(sizeof(vg->val_len) == 1);
+            *p++ = (char) vg->val_len;
+            if ( vg->key_len )

You ASSERTed that vg->key_len is 1 so you may not need the 'if'. (And in NDEBUG version the worst that could happen is you do memcpy(,,0), which is safe).


You seem to have missed the sizeof() in there. So vg->key_len can be zero. But I will agree
that the if could be dropped.

+            {
+                memcpy(p, vg->key_data, vg->key_len);
+                p += vg->key_len;
+                if ( vg->val_len )

Same here.


Yes, the if is not needed.  Can drop if requested.

+                {
+                    memcpy(p, vg->val_data, vg->val_len);
+                    p += vg->val_len;
+                }
+            }
+        }
+    }
+    ctxt->used_guestinfo = used_guestinfo;
+
+    for ( i = 0; i < ctxt->used_guestinfo_jumbo; i++ )
+    {
+        vmport_guestinfo_jumbo_t *vgj =
+            vs->guestinfo_jumbo[i];
+        if ( vgj && vgj->key_len )
+        {
+ guestinfo_size += sizeof(vgj->key_len) + sizeof(vgj->val_len) +
+                vgj->key_len + vgj->val_len;
+            used_guestinfo_jumbo++;
+            ASSERT(sizeof(vgj->key_len) == 1);
+            *p++ = (char) vgj->key_len;
+            memcpy(p, &vgj->val_len, sizeof(vgj->val_len));
+            p += sizeof(vgj->val_len);
+            if ( vgj->key_len )

And here.

And do you need ASSERT(vgj->val_len ==1) as well? Also, In vmport_load_domain_ctxt() you don't seem to be making this assertions. I don't know whether this is on purpose.


In this case sizeof(vgj->val_len) != 1.

I will add ASSERTs there also.




+
+    if ( !vs )
+        return -ENOMEM;
+
+ /* Customized checking for entry since our entry is of variable length */
+    desc = (struct hvm_save_descriptor *)&h->data[h->cur];
+    if ( sizeof(*desc) > h->size - h->cur )
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d restore: not enough data left to read descriptor"
+               "for type %lu\n", d->domain_id,
+               HVM_SAVE_CODE(VMPORT));
+        return -1;

Since in some cases you are returning proper error codes you should return them everywhere.


Will adjust to proper error codes.

   -Don Slutz

-boris




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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.