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;