On 10/16/2015 08:11 AM, Peter Krempa wrote:
> Aggregate the checks of the dimm device into the verification function
> rather than having them in the formatter.
> ---
> src/qemu/qemu_command.c | 65 ++------------------------------------
> src/qemu/qemu_command.h | 4 +--
> src/qemu/qemu_domain.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_hotplug.c | 2 +-
> 4 files changed, 86 insertions(+), 68 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4a709db..5e7b052 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -5318,44 +5318,8 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
> }
>
>
> -static bool
> -qemuCheckMemoryDimmConflict(virDomainDefPtr def,
> - virDomainMemoryDefPtr mem)
> -{
> - size_t i;
> -
> - 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' is already being "
> - "used by another memory device"),
> - mem->info.addr.dimm.slot);
> - return true;
> - }
> -
> - if (mem->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' is already being "
> - "used by another memory device"),
> - mem->info.addr.dimm.base);
> - return true;
> - }
> - }
> -
> - return false;
> -}
> -
> char *
> -qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> - virDomainDefPtr def,
> - virQEMUCapsPtr qemuCaps)
> +qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> @@ -5367,35 +5331,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
>
> switch ((virDomainMemoryModel) mem->model) {
> case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) {
> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> - _("this qemu doesn't support the pc-dimm device"));
> - 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) {
> - if (qemuCheckMemoryDimmConflict(def, mem))
> - return NULL;
> -
> virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot);
> virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> }
> @@ -9486,7 +9425,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> qemuCaps, cfg)))
> goto error;
>
> - if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i], def, qemuCaps))) {
> + if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i]))) {
> VIR_FREE(backStr);
> goto error;
> }
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 4aa7f2d..c7ed300 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -173,9 +173,7 @@ int qemuBuildMemoryBackendStr(unsigned long long size,
> virJSONValuePtr *backendProps,
> bool force);
>
> -char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> - virDomainDefPtr def,
> - virQEMUCapsPtr qemuCaps);
> +char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem);
>
> /* Legacy, pre device support */
> char *qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index cb50754..1474a5b 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -3552,6 +3552,79 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def)
> }
>
>
> +static bool
> +qemuCheckMemoryDimmConflict(const virDomainDef *def,
> + const virDomainMemoryDef *mem)
> +{
> + size_t i;
> +
> + 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' is already being "
> + "used by another memory device"),
> + mem->info.addr.dimm.slot);
> + return true;
> + }
> +
> + if (mem->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' is already being "
> + "used by another memory device"),
> + mem->info.addr.dimm.base);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +static int
> +qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem,
> + const virDomainDef *def)
> +{
> + switch ((virDomainMemoryModel) mem->model) {
> + case VIR_DOMAIN_MEMORY_MODEL_DIMM:
> + 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 -1;
> + }
> +
> + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> + 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;
> + }
> +
> +
> + if (qemuCheckMemoryDimmConflict(def, mem))
> + return -1;
> + }
> + break;
> +
> + case VIR_DOMAIN_MEMORY_MODEL_NONE:
> + case VIR_DOMAIN_MEMORY_MODEL_LAST:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid memory device type"));
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> /**
> * qemuDomainDefValidateMemoryHotplug:
> * @def: domain definition
> @@ -3616,9 +3689,17 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def,
> return -1;
> }
>
> - for (i = 0; i < def->nmems; i++)
> + for (i = 0; i < def->nmems; i++) {
> hotplugMemory += def->mems[i]->size;
>
> + if (qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 0)
> + return -1;
If we are in the hotplug path (qemuDomainAttachMemory), then this is
duplicitous. IOW: Why check something we've already validated when we
started? In this case "if (!mem &&" would reduce the extra check, but
perhaps be confusing or strange to read.
> + }
> +
> + if (mem &&
> + qemuDomainDefValidateMemoryHotplugDevice(mem, def) < 0)
> + return -1;
> +
In my mind - this would be cleaner if combined with the above first "if
(mem)" condition... Your call on moving it though... at least the check
is there.
ACK - although I do think the tweak to not validate the already
validated would be appropriate.
John
> if (hotplugMemory > hotplugSpace) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> _("memory device total size exceeds hotplug space"));
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 2c7f165..d3630d7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1796,7 +1796,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def))
> fix_balloon = true;
>
> - if (!(devstr = qemuBuildMemoryDeviceStr(mem, vm->def, priv->qemuCaps)))
> + if (!(devstr = qemuBuildMemoryDeviceStr(mem)))
> goto cleanup;
>
> qemuDomainMemoryDeviceAlignSize(vm->def, mem);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list