On 02/27/2017 08:19 AM, Michal Privoznik wrote:
> Frankly, this function is one big mess. A lot of arguments,
> complicated behaviour. It's really surprising that arguments were
> in random order (input and output arguments were mixed together),
> the documentation was outdated, the description of return values
> was bogus.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 64 +++++++++++++++++++++++++++----------------------
> src/qemu/qemu_command.h | 14 +++++------
> src/qemu/qemu_hotplug.c | 7 +++---
> 3 files changed, 46 insertions(+), 39 deletions(-)
>
Why not have qemuBuildMemoryBackendStr take a virDomainMemoryDefPtr? In
qemuBuildMemoryCellBackendStr you could create one on the stack
(virDomainMemoryDef mem = {0};) filling in memsize and cell
appropriately before calling... Probably makes a subsequent patch
easier too...
Perhaps a multistep process to remove the "extra" args and pass the
pointer instead... Then alter the order of the args afterwards. Although
I think perhaps the current "force" is misnamed and could be a output too...
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 41eecfd18..7c52712d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3077,38 +3077,47 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
>
> /**
> * qemuBuildMemoryBackendStr:
> + * @backendProps: [out] constructed object
> + * @backendType: [out] type of the backennd used
s/backennd/backend
Why have [out] parameters first and not last? Sure that nasty force
flag is at the end now, but that could move. Not that we have a standard
for this ~/~ yet...
> + * @cfg: qemu driver config object
> + * @qemuCaps: qemu capabilities object
> + * @def: domain definition object
> * @size: size of the memory device in kibibytes
> * @pagesize: size of the requested memory page in KiB, 0 for default
> * @guestNode: NUMA node in the guest that the memory object will be attached
> * to, or -1 if NUMA is not used in the guest
> - * @hostNodes: map of host nodes to alloc the memory in, NULL for default
> - * @autoNodeset: fallback nodeset in case of automatic numa placement
> - * @def: domain definition object
> - * @qemuCaps: qemu capabilities object
> - * @cfg: qemu driver config object
> - * @aliasPrefix: prefix string of the alias (to allow for multiple frontents)
> - * @id: index of the device (to construct the alias)
> - * @backendStr: returns the object string
> + * @userNodeset: user requested map of host nodes to alloc the memory on, NULL
> + * for default
> + * @autoNodeset: fallback nodeset in case of automatic NUMA placement
> + * @force: forcibly use one of the backends
I see that commit '1c4f3b5' allows the code to change "force" from it's
passed value (from false to true)... Really makes things quite ugly...
and it seems about to get even uglier.
That seemingly would only affect qemuBuildMemoryCellBackendStr since
it's the only one passing 'false', but does alter some of the following
description.
> *
> - * Formats the configuration string for the memory device backend according
> - * to the configuration. @pagesize and @hostNodes can be used to override the
> - * default source configuration, both are optional.
> + * Creates a configuration object that represents memory backend of given guest
> + * NUMA node (domain @def and @guestNode) of size @size. @pagesize can be used
> + * to override configured size of hugepages. Use @userNodeset and @autoNodeset
> + * to fine tune the placement of the memory on the host NUMA nodes.
> *
> - * Returns 0 on success, 1 if only the implicit memory-device-ram with no
> - * other configuration was used (to detect legacy configurations). Returns
> - * -1 in case of an error.
> + * By default, if no memory-backend-* object is necessary to fulfil the guest
> + * configuration value of 1 is returned. This behaviour can be suppressed by
> + * setting @force to true in which case 0 would be returned.
> + *
> + * Then, if one of the two memory-backend-* should be used, the @qemuCaps is
> + * consulted to check if qemu does support it.
> + *
> + * Returns: 0 on success,
> + * 1 on success and if there's no need to use memory-backend-*
> + * -1 on error.
> */
Perhaps the name 'force' is bad and could be turned into an output value
too.... That way it's a 0 or -1 return value and there's a "bool
*need_backend;"... I'm sure the logic could be reworked to check if
someone cares (e.g. a NULL could be passed) and if they do care, then
set the value based on whether or not it's needed.
One other NIT with the existing code... there's a check "&& !memAccess"
of course that should be (memAccess != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)...
John
> int
> -qemuBuildMemoryBackendStr(unsigned long long size,
> - unsigned long long pagesize,
> +qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> + const char **backendType,
> + virQEMUDriverConfigPtr cfg,
> + virQEMUCapsPtr qemuCaps,
> + virDomainDefPtr def,
> int guestNode,
> + unsigned long long size,
> + unsigned long long pagesize,
> virBitmapPtr userNodeset,
> virBitmapPtr autoNodeset,
> - virDomainDefPtr def,
> - virQEMUCapsPtr qemuCaps,
> - virQEMUDriverConfigPtr cfg,
> - const char **backendType,
> - virJSONValuePtr *backendProps,
> bool force)
> {
> virDomainHugePagePtr master_hugepage = NULL;
> @@ -3327,9 +3336,9 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def,
> if (virAsprintf(&alias, "ram-node%zu", cell) < 0)
> goto cleanup;
>
> - if ((rc = qemuBuildMemoryBackendStr(memsize, 0, cell, NULL, auto_nodeset,
> - def, qemuCaps, cfg, &backendType,
> - &props, false)) < 0)
> + if ((rc = qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps,
> + def, cell, memsize, 0, NULL,
> + auto_nodeset, false)) < 0)
> goto cleanup;
>
> if (!(*backendStr = virQEMUBuildObjectCommandlineFromJSON(backendType,
> @@ -3368,10 +3377,9 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
> if (virAsprintf(&alias, "mem%s", mem->info.alias) < 0)
> goto cleanup;
>
> - if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize,
> - mem->targetNode, mem->sourceNodes, auto_nodeset,
> - def, qemuCaps, cfg,
> - &backendType, &props, true) < 0)
> + if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, qemuCaps, def,
> + mem->targetNode, mem->size, mem->pagesize,
> + mem->sourceNodes, auto_nodeset, true) < 0)
> goto cleanup;
>
> ret = virQEMUBuildObjectCommandlineFromJSON(backendType, alias, props);
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 69fe84613..efdad77f3 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -124,16 +124,16 @@ char *qemuBuildControllerDevStr(const virDomainDef *domainDef,
> virQEMUCapsPtr qemuCaps,
> int *nusbcontroller);
>
> -int qemuBuildMemoryBackendStr(unsigned long long size,
> - unsigned long long pagesize,
> +int qemuBuildMemoryBackendStr(virJSONValuePtr *backendProps,
> + const char **backendType,
> + virQEMUDriverConfigPtr cfg,
> + virQEMUCapsPtr qemuCaps,
> + virDomainDefPtr def,
> int guestNode,
> + unsigned long long size,
> + unsigned long long pagesize,
> virBitmapPtr userNodeset,
> virBitmapPtr autoNodeset,
> - virDomainDefPtr def,
> - virQEMUCapsPtr qemuCaps,
> - virQEMUDriverConfigPtr cfg,
> - const char **backendType,
> - virJSONValuePtr *backendProps,
> bool force);
>
> char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 97fb272f6..c7b8074d6 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2216,10 +2216,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver,
> if (!(devstr = qemuBuildMemoryDeviceStr(mem)))
> goto cleanup;
>
> - if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize,
> - mem->targetNode, mem->sourceNodes, NULL,
> - vm->def, priv->qemuCaps, cfg,
> - &backendType, &props, true) < 0)
> + if (qemuBuildMemoryBackendStr(&props, &backendType, cfg, priv->qemuCaps,
> + vm->def, mem->targetNode, mem->size,
> + mem->pagesize, mem->sourceNodes, NULL, true) < 0)
> goto cleanup;
>
> if (virDomainMemoryInsert(vm->def, mem) < 0) {
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
