[libvirt] [PATCHv2] qemu: add a check for slot and base when build dimm address
When hot-plug a memory device, we don't check if there is a memory device have the same address with the memory device we want hot-pluged. Qemu forbid use/hot-plug 2 memory device with same slot or the same base(qemu side this elemnt named addr). Introduce a address check when build memory device qemu command line. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: move the check to qemuBuildMemoryDeviceStr() to check the dimm address when start/hot-plug a memory device. src/qemu/qemu_command.c | 77 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d8ce511..0380a3b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, } +static int +qemuBuildMemoryDeviceAddr(virBuffer *buf, + virDomainDefPtr def, + virDomainMemoryDefPtr mem) +{ + ssize_t i; + + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'dimm' addresses are supported for the " + "pc-dimm device")); + return -1; + } + + if (mem->info.addr.dimm.slot >= def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' exceeds slots count '%u'"), + mem->info.addr.dimm.slot, def->mem.memory_slots); + return -1; + } + + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (tmp == mem || + tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + continue; + + if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' already been" + " used by other memory device"), + mem->info.addr.dimm.slot); + return -1; + } + + if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 && + mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device base '0x%llx' already been" + " used by other memory device"), + mem->info.addr.dimm.base); + return -1; + } + } + + virBufferAsprintf(buf, ",slot=%d", mem->info.addr.dimm.slot); + virBufferAsprintf(buf, ",addr=%llu", mem->info.addr.dimm.base); + + return 0; +} + char * qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, virDomainDefPtr def, @@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, return NULL; } - if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only 'dimm' addresses are supported for the " - "pc-dimm device")); - return NULL; - } - - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.addr.dimm.slot >= def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%u' exceeds slots count '%u'"), - mem->info.addr.dimm.slot, def->mem.memory_slots); - return NULL; - } - virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s", mem->targetNode, mem->info.alias, mem->info.alias); - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base); - } + if (qemuBuildMemoryDeviceAddr(&buf, def, mem) < 0) + return NULL; break; -- 1.8.3.1
On 05/27/2015 05:50 AM, Luyao Huang wrote:
When hot-plug a memory device, we don't check if there is a memory device have the same address with the memory device we want hot-pluged. Qemu forbid use/hot-plug 2 memory device with same slot or the same base(qemu side this elemnt named addr).
Introduce a address check when build memory device qemu command line.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: move the check to qemuBuildMemoryDeviceStr() to check the dimm address when start/hot-plug a memory device.
src/qemu/qemu_command.c | 77 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 20 deletions(-)
Perhaps a bit easier to review if this was split into two patches the first being purely code motion and the second being fixing the problem... That said, I don't think the refactor is necessary. I've attached an alternative and some notes inline below... John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d8ce511..0380a3b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, }
+static int +qemuBuildMemoryDeviceAddr(virBuffer *buf, + virDomainDefPtr def, + virDomainMemoryDefPtr mem)
static bool qemuCheckMemoryDimmConflict(virDomainDefPtr def, virDomainMemoryDefPtr mem)
+{ + ssize_t i;
size_t usually, then keep the following checks in the caller
+ + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'dimm' addresses are supported for the " + "pc-dimm device")); + return -1; + } + + if (mem->info.addr.dimm.slot >= def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' exceeds slots count '%u'"), + mem->info.addr.dimm.slot, def->mem.memory_slots); + return -1; + } +
Thru here... Since it seems the following is your bugfix correct?
+ for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (tmp == mem || + tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + continue; + + if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' already been" + " used by other memory device"),
...is already being used by another
+ mem->info.addr.dimm.slot); + return -1;
return true;
+ } + + if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 && + mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device base '0x%llx' already been"
...is already being used by another...
+ " used by other memory device"), + mem->info.addr.dimm.base); + return -1;
return true
+ } + }
return false; Keep remainder in caller
+ + virBufferAsprintf(buf, ",slot=%d", mem->info.addr.dimm.slot); + virBufferAsprintf(buf, ",addr=%llu", mem->info.addr.dimm.base); + + return 0; +} + char * qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, virDomainDefPtr def, @@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, return NULL; }
- if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only 'dimm' addresses are supported for the " - "pc-dimm device"));
Your refactor adjusts this test, so if the type was something else then the virBufferAsprintf happens before the error... it's a nit, but future adjustments could do something unexpected...
- return NULL; - } - - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.addr.dimm.slot >= def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%u' exceeds slots count '%u'"), - mem->info.addr.dimm.slot, def->mem.memory_slots); - return NULL; - } -
Rather than refactoring - why not change the purpose of the called routine... if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && qemuCheckMemoryDimmConflict(def, mem)) return NULL;
virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s", mem->targetNode, mem->info.alias, mem->info.alias);
- if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base); - } + if (qemuBuildMemoryDeviceAddr(&buf, def, mem) < 0) + return NULL;
break;
On 06/11/2015 03:12 AM, John Ferlan wrote:
On 05/27/2015 05:50 AM, Luyao Huang wrote:
When hot-plug a memory device, we don't check if there is a memory device have the same address with the memory device we want hot-pluged. Qemu forbid use/hot-plug 2 memory device with same slot or the same base(qemu side this elemnt named addr).
Introduce a address check when build memory device qemu command line.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- v2: move the check to qemuBuildMemoryDeviceStr() to check the dimm address when start/hot-plug a memory device.
src/qemu/qemu_command.c | 77 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 20 deletions(-)
Perhaps a bit easier to review if this was split into two patches the first being purely code motion and the second being fixing the problem... That said, I don't think the refactor is necessary. I've attached an alternative and some notes inline below...
Yes, i should split these changes in two patches, however i think you are right i will remove the unnecessary refactor in next version, so no need split ;)
John
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d8ce511..0380a3b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, }
+static int +qemuBuildMemoryDeviceAddr(virBuffer *buf, + virDomainDefPtr def, + virDomainMemoryDefPtr mem) static bool qemuCheckMemoryDimmConflict(virDomainDefPtr def, virDomainMemoryDefPtr mem)
+{ + ssize_t i; size_t usually, then keep the following checks in the caller
Okay
+ + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'dimm' addresses are supported for the " + "pc-dimm device")); + return -1; + } + + if (mem->info.addr.dimm.slot >= def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' exceeds slots count '%u'"), + mem->info.addr.dimm.slot, def->mem.memory_slots); + return -1; + } + Thru here... Since it seems the following is your bugfix correct?
Yes, this is bugfix part
+ for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (tmp == mem || + tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + continue; + + if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' already been" + " used by other memory device"), ...is already being used by another
+ mem->info.addr.dimm.slot); + return -1; return true;
+ } + + if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 && + mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device base '0x%llx' already been" ...is already being used by another...
Thanks for pointing out this.
+ " used by other memory device"), + mem->info.addr.dimm.base); + return -1; return true
+ } + } return false;
Keep remainder in caller
+ + virBufferAsprintf(buf, ",slot=%d", mem->info.addr.dimm.slot); + virBufferAsprintf(buf, ",addr=%llu", mem->info.addr.dimm.base); + + return 0; +} + char * qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, virDomainDefPtr def, @@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, return NULL; }
- if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only 'dimm' addresses are supported for the " - "pc-dimm device")); Your refactor adjusts this test, so if the type was something else then the virBufferAsprintf happens before the error... it's a nit, but future adjustments could do something unexpected...
Right, i forgot this :)
- return NULL; - } - - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.addr.dimm.slot >= def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%u' exceeds slots count '%u'"), - mem->info.addr.dimm.slot, def->mem.memory_slots); - return NULL; - } - Rather than refactoring - why not change the purpose of the called routine...
if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && qemuCheckMemoryDimmConflict(def, mem)) return NULL;
Yes, the refactor is unnecessary. I just thought i should move all the check for pc-dimm address in another function to check and build the address and make the code more clean (because after introduce my bugfix part in qemuBuildMemoryDeviceStr() function the code looks bloated and ugly). Thanks for your review and i will write a version these days.
virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s", mem->targetNode, mem->info.alias, mem->info.alias);
- if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base); - } + if (qemuBuildMemoryDeviceAddr(&buf, def, mem) < 0) + return NULL;
break;
Luyao
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang