On 03/04/2015 11:24 AM, Peter Krempa wrote:
> Add a few helpers that allow to operate with memory device definitions
> on the domain config and use them to implement memory device coldplug in
> the qemu driver.
> ---
> src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 10 +++++
> src/libvirt_private.syms | 4 ++
> src/qemu/qemu_driver.c | 15 ++++++-
> 4 files changed, 127 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4f20aa6..0f6058b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12687,6 +12687,106 @@ virDomainRNGRemove(virDomainDefPtr def,
> }
>
>
> +static int
> +virDomainMemoryFindByDefInternal(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem,
> + bool allowAddressFallback)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nmems; i++) {
> + virDomainMemoryDefPtr tmp = def->mems[i];
> +
> + /* address, if present */
> + if (allowAddressFallback) {
> + if (tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> + continue;
So this path says - we want address fallback and this device has either
DIMM or LAST (oy!) as a type, so go to the next one and ignore this one
> + } else {
> + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> + !virDomainDeviceInfoAddressIsEqual(&tmp->info, &mem->info))
> + continue;
This path says - we don't want address fallback and as long as we're
DIMM or LAST (oy!), then compare
What happens when we add a new address type? It would seem the more
straightforward way would be
if (type == DIMM && !Equal)
if (!allowAddressFallback)
continue'
Otherwise we're falling through to alias, target, etc. checks
> + }
> +
> + /* alias, if present */
> + if (mem->info.alias &&
> + STRNEQ_NULLABLE(tmp->info.alias, mem->info.alias))
> + continue;
I thought the NULLABLE checks both elems for NULL...
> +
> + /* target info -> always present */
> + if (tmp->model != mem->model ||
> + tmp->targetNode != mem->targetNode ||
> + tmp->size != mem->size)
> + continue;
> +
> + /* source stuff -> match with device */
> + if (tmp->pagesize != mem->pagesize)
> + continue;
> +
> + if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes))
> + continue;
> +
> + break;
> + }
> +
> + if (i == def->nmems)
> + return -1;
> +
> + return i;
> +}
> +
> +
> +int
> +virDomainMemoryFindByDef(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> + return virDomainMemoryFindByDefInternal(def, mem, false);
> +}
> +
> +
> +int
> +virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> + int ret;
> +
> + if ((ret = virDomainMemoryFindByDefInternal(def, mem, false)) < 0)
> + ret = virDomainMemoryFindByDefInternal(def, mem, true);
I would seem Inactive would probably not have the dimm address set, so
we're incurring a 2X penalty for perhaps no reason... Unless perhaps the
incoming mem def being checked has an address...
That is if my incoming has an address, then don't allow fallback;
otherwise, well best match.
> +
> + return ret;
> +}
> +
> +
> +int
> +virDomainMemoryInsert(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> +{
> + int id = def->nmems;
> +
> + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> + virDomainDefHasDeviceAddress(def, &mem->info)) {
Hmm... so if our incoming mem has an address defined - it could be
anything - we're just failing declaring that the domain already contains
a device with the same address? Doesn't seem right.
And again - we have this problem of TYPE_NONE, TYPE_DIMM, TYPE_LAST and
who knows what in the future.
> + virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Domain already contains a device with the same "
> + "address"));
> + return -1;
> + }
> +
> + if (VIR_APPEND_ELEMENT(def->mems, def->nmems, mem) < 0)
> + return -1;
> +
> + return id;
> +}
> +
> +
> +virDomainMemoryDefPtr
> +virDomainMemoryRemove(virDomainDefPtr def,
> + int idx)
> +{
> + virDomainMemoryDefPtr ret = def->mems[idx];
> + VIR_DELETE_ELEMENT(def->mems, idx, def->nmems);
> + return ret;
> +}
> +
> +
> char *
> virDomainDefGetDefaultEmulator(virDomainDefPtr def,
> virCapsPtr caps)
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 706040d..c38614d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2853,6 +2853,16 @@ virDomainChrDefGetSecurityLabelDef(virDomainChrDefPtr def, const char *model);
> typedef const char* (*virEventActionToStringFunc)(int type);
> typedef int (*virEventActionFromStringFunc)(const char *type);
>
> +int virDomainMemoryInsert(virDomainDefPtr def, virDomainMemoryDefPtr mem)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +virDomainMemoryDefPtr virDomainMemoryRemove(virDomainDefPtr def, int idx)
> + ATTRIBUTE_NONNULL(1);
> +int virDomainMemoryFindByDef(virDomainDefPtr def, virDomainMemoryDefPtr mem)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +int virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
> + virDomainMemoryDefPtr mem)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> +
> VIR_ENUM_DECL(virDomainTaint)
> VIR_ENUM_DECL(virDomainVirt)
> VIR_ENUM_DECL(virDomainBoot)
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 2491d2d..1504c9a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -334,6 +334,10 @@ virDomainLockFailureTypeToString;
> virDomainMemballoonModelTypeFromString;
> virDomainMemballoonModelTypeToString;
> virDomainMemoryDefFree;
> +virDomainMemoryFindByDef;
> +virDomainMemoryFindInactiveByDef;
> +virDomainMemoryInsert;
> +virDomainMemoryRemove;
> virDomainNetAppendIpAddress;
> virDomainNetDefFormat;
> virDomainNetDefFree;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d6935d9..6b10e96 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7438,7 +7438,10 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> break;
>
> case VIR_DOMAIN_DEVICE_MEMORY:
> - /* TODO: implement later */
> + if (virDomainMemoryInsert(vmdef, dev->data.memory) < 0)
> + return -1;
> + dev->data.memory = NULL;
> + break;
>
> case VIR_DOMAIN_DEVICE_INPUT:
> case VIR_DOMAIN_DEVICE_SOUND:
> @@ -7566,7 +7569,15 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> break;
>
> case VIR_DOMAIN_DEVICE_MEMORY:
> - /* TODO: implement later */
> + if ((idx = virDomainMemoryFindInactiveByDef(vmdef,
> + dev->data.memory)) < 0) {
> + virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("matching memory device was not found"));
> + return -1;
> + }
> +
> + virDomainMemoryDefFree(virDomainMemoryRemove(vmdef, idx));
Ug - tricked my eyes on calling the Remove inside the DefFree
ACK with some cleanups which I think are more or less simple
John
I have to run - I will finish 9 & 10 a bit later.
> + break;
>
> case VIR_DOMAIN_DEVICE_INPUT:
> case VIR_DOMAIN_DEVICE_SOUND:
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list