[libvirt] [PATCH 0/2] Add mem cold/hot plug supported check to virDomainDefCompatibleDevice
https://bugzilla.redhat.com/show_bug.cgi?id=1624336 Details are in patch2, but essentially the issue is the check for whether cold/hot plug of memory is supported occurs during qemuDomainDefValidateMemoryHotplug; however, that is called after virDomainDefCompatibleDevice, but is not called during the qemuDomainAttachDeviceConfig processing. Another solution would be to modify virDomainDefCompatibleDevice to add a "if (def->mem.max_memory > 0 &&" check before the check for whether the size fits and virDomainDefHasMemoryHotplug could be called from qemuDomainAttachDeviceConfig, but that seems a bit strange. The additional call is to avoid the equally strange message that would appear "no free memory device slot available" because nmems == mem.memory_slots == 0. If this solution is preferred I'm fine with that, but figured I needed to start somewhere. John Ferlan (2): conf: Add the size of failed max_memory in error conf: Add check/error for domain supports cold/hotplug src/conf/domain_conf.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) -- 2.17.2
If virDomainDefCompatibleDevice fails because there is insufficient domain def->mem.max_memory, then let's also print out that value in the error message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b70dca6c61..2a17214f38 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28901,7 +28901,8 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, if ((virDomainDefGetMemoryTotal(def) + sz) > def->mem.max_memory) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Attaching memory device with size '%llu' would " - "exceed domain's maxMemory config"), sz); + "exceed domain's maxMemory config size '%llu'"), + sz, def->mem.max_memory); return -1; } } -- 2.17.2
https://bugzilla.redhat.com/show_bug.cgi?id=1624336 Add a check during virDomainDefCompatibleDevice whether the domain supports cold/hotplug of a memory module even though this duplicates the qemuDomainDefValidateMemoryHotplug check. Without this check, the cold/hot plug would fail on the subsequent mem_memory check (since it's 0). Adding a check for max_memory > 0 would allow the subsequent hotplug check to fail, but would cause coldplug to fail with the somewhat opaque message "no free memory device slot available". Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a17214f38..b83368a43a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28898,6 +28898,13 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, if (dev->type == VIR_DOMAIN_DEVICE_MEMORY) { unsigned long long sz = dev->data.memory->size; + if (!virDomainDefHasMemoryHotplug(def)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot use/hotplug a memory device when domain " + "'maxMemory' is not defined")); + return -1; + } + if ((virDomainDefGetMemoryTotal(def) + sz) > def->mem.max_memory) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Attaching memory device with size '%llu' would " -- 2.17.2
On 12/7/18 6:27 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624336
Details are in patch2, but essentially the issue is the check for whether cold/hot plug of memory is supported occurs during qemuDomainDefValidateMemoryHotplug; however, that is called after virDomainDefCompatibleDevice, but is not called during the qemuDomainAttachDeviceConfig processing.
Another solution would be to modify virDomainDefCompatibleDevice to add a "if (def->mem.max_memory > 0 &&" check before the check for whether the size fits and virDomainDefHasMemoryHotplug could be called from qemuDomainAttachDeviceConfig, but that seems a bit strange. The additional call is to avoid the equally strange message that would appear "no free memory device slot available" because nmems == mem.memory_slots == 0. If this solution is preferred I'm fine with that, but figured I needed to start somewhere.
John Ferlan (2): conf: Add the size of failed max_memory in error conf: Add check/error for domain supports cold/hotplug
src/conf/domain_conf.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
ACK Michal
participants (2)
-
John Ferlan -
Michal Privoznik