On 03/07/2017 05:00 PM, John Ferlan wrote:
>
>
> 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...
Sure. Will work on that.
>
> Perhaps a multistep process to remove the "extra" args and pass the
> pointer instead... Then alter the order of the args afterwards.
Agreed.
> Although
> I think perhaps the current "force" is misnamed and could be a output too...
Not quite sure why we want to do that.
>
>
>> 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...
I guess this an old habit from glib world. For instance:
http://libvirt.org/git/?p=libvirt-glib.git;a=blob;f=libvirt-gobject/libvirt-gobject-domain.c;h=7be936db21d8bfeaa16f6b08097029f52e501902;hb=HEAD#l1787
>
>
>> + * @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.
Yeah, I have a fix for that too.
>
> 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.
Not quite sure why we should do this. Lets save that for a follow up patch.
>
> One other NIT with the existing code... there's a check "&& !memAccess"
> of course that should be (memAccess != VIR_DOMAIN_MEMORY_ACCESS_DEFAULT)...
Okay, will fix that as well.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
