[libvirt] [PATCH 04/10] qemu: command: Always execute memory device formatter

[John Ferlan]

On 10/16/2015 08:11 AM, Peter Krempa wrote:
> Since we already make sure before that the domain configuration is
> valid we may execute it always at the cost of doing 0 iterations of the
> for loop.
> 
> This patch will simplify later refactor as it will avoid whitespace
> changes.
> ---
>  src/qemu/qemu_command.c | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a37a4fa..43e81c7 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9484,38 +9484,37 @@ qemuBuildCommandLine(virConnectPtr conn,
>          }
>      }
> 
> -    if (virDomainNumaGetNodeCount(def->numa)) {
> -        if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0)
> +    if (virDomainNumaGetNodeCount(def->numa) &&
> +        qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0)
>              goto error;
> 
> -        /* memory hotplug requires NUMA to be enabled - we already checked
> -         * that memory devices are present only when NUMA is */
> +    /* memory hotplug requires NUMA to be enabled - we already checked
> +     * that memory devices are present only when NUMA is */

The second half of this comment made me wonder where...  especially with
the removal of the virDomainNumaGetNodeCount call which would
essentially be checking if numa was enabled (I think).

The virDomainDefHasMemoryHotplug checks memory_slots or max_memory

The virDomainDefPostParseMemory could have it, but it's not overt

Not that the check is wrong, but it would be nice to fixup the comment
to indicate that 'xxx' checked that memory devices are present only when
NUMA is.  That at least helps ensure any future refactor doesn't impact
the assumption made here.

ACK with that

John
> 
> -        if (def->nmems > def->mem.memory_slots) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("memory device count '%zu' exceeds slots count '%u'"),
> -                           def->nmems, def->mem.memory_slots);
> -            goto error;
> -        }
> -
> -        for (i = 0; i < def->nmems; i++) {
> -            char *backStr;
> -            char *dimmStr;
> -
> -            if (!(backStr = qemuBuildMemoryDimmBackendStr(def->mems[i], def,
> -                                                          qemuCaps, cfg)))
> -                goto error;
> +    if (def->nmems > def->mem.memory_slots) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("memory device count '%zu' exceeds slots count '%u'"),
> +                       def->nmems, def->mem.memory_slots);
> +        goto error;
> +    }
> 
> -            if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i], def, qemuCaps))) {
> -                VIR_FREE(backStr);
> -                goto error;
> -            }
> +    for (i = 0; i < def->nmems; i++) {
> +        char *backStr;
> +        char *dimmStr;
> 
> -            virCommandAddArgList(cmd, "-object", backStr, "-device", dimmStr, NULL);
> +        if (!(backStr = qemuBuildMemoryDimmBackendStr(def->mems[i], def,
> +                                                      qemuCaps, cfg)))
> +            goto error;
> 
> +        if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i], def, qemuCaps))) {
>              VIR_FREE(backStr);
> -            VIR_FREE(dimmStr);
> +            goto error;
>          }
> +
> +        virCommandAddArgList(cmd, "-object", backStr, "-device", dimmStr, NULL);
> +
> +        VIR_FREE(backStr);
> +        VIR_FREE(dimmStr);
>      }
> 
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_UUID))
>