Re: [PATCHv3 10/12] qemu: conf: Add support for memory device cold(un)plug

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

 



On Fri, Mar 20, 2015 at 07:40:21 -0400, John Ferlan wrote:
> 
> >>> I have tested your series with our qemu memory hot remove patch series,
> >>> here would be a possible error.
> >>>
> >>> When hotplug a memory device, its size has been aligned. So the
> >>> compare for
> >>> size here would fail possiblely.
> >>>
> >> hmm.. Not sure that's necessary - although Peter can make the final
> >> determination... Commit id '57b215a' doesn't modify each def->mems[i]
> >> entry in qemuDomainAlignMemorySizes, rather it gets a value from
> >> virDomainDefSetMemoryInitial and then does the rounding.
> >>
> >> If the stored def->mems[i]->size value is/was modified, then I'd agree,
> >> but it doesn't appear to be that way.
> >>
> >> If there is a rounding of the value - then please just point it out
> > 
> > Yes, the stored def->mems[i]->size value was modified.
> > If you assign the size 524287 KiB, the stored value will be 524288.
> > 
> > Thanks,
> > Zhu
> > 
> 
> Ah - found it - patch 9 has:
> 
> +    /* Align memory module sizes */
> +    for (i = 0; i < def->nmems; i++)
> +        qemuDomainMemoryDeviceAlignSize(def->mems[i]);
> +
> 
> Which I missed on my first foray through this. Once I cscope'd on
> VIR_ROUND_UP() instead of ->size, it became apparent
> 
> So yes, it seems the to be compared size needs a likewise adjustment.

Indeed, but the size needs to be aligned only for the active definition
as we only align that one, thus it belongs to patch 12/12.

I'll be adding the following diff to 12/12:

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 40041d5..9b8d11b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4189,6 +4189,8 @@ qemuDomainDetachMemoryDevice(virQEMUDriverPtr driver,
         return -1;
     }

+    qemuDomainMemoryDeviceAlignSize(memdef);
+
     if ((idx = virDomainMemoryFindByDef(vm->def, memdef)) < 0) {
         virReportError(VIR_ERR_OPERATION_INVALID, "%s",
                        _("device not present in domain configuration"));

Peter

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]