10:27 p.m.
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(a)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