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