[libvirt] [PATCH v2 01/14] qemuBuildMemoryBackendStr: Reorder args and update comment

[Michal Privoznik]

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 at redhat.com>
>> ---
>>  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