[libvirt] [PATCH v3 06/20] Make qemuCapsProbeMachineTypes & qemuCapsProbeCPUModels static

[Jiri Denemark]

On Tue, Sep 25, 2012 at 18:59:59 +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> The qemuCapsProbeMachineTypes & qemuCapsProbeCPUModels methods
> do not need to be invoked directly anymore. Make them static
> and refactor them to directly populate the qemuCapsPtr object
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c | 238 ++++++++++++++++---------------------------
>  src/qemu/qemu_capabilities.h |  13 +--
>  2 files changed, 90 insertions(+), 161 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 888c56c..7e4ea50 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -305,17 +305,16 @@ qemuCapsProbeCommand(const char *qemu,
>   */
>  static int
>  qemuCapsParseMachineTypesStr(const char *output,
> -                             virCapsGuestMachinePtr **machines,
> -                             size_t *nmachines)
> +                             qemuCapsPtr caps)
>  {
>      const char *p = output;
>      const char *next;
> -    virCapsGuestMachinePtr *list = NULL;
> -    int nitems = 0;
> +    size_t defIdx = 0;
>  
>      do {
>          const char *t;
> -        virCapsGuestMachinePtr machine;
> +        char *name;
> +        char *canonical = NULL;
>  
>          if ((next = strchr(p, '\n')))
>              ++next;
> @@ -326,56 +325,61 @@ qemuCapsParseMachineTypesStr(const char *output,
>          if (!(t = strchr(p, ' ')) || (next && t >= next))
>              continue;
>  
> -        if (VIR_ALLOC(machine) < 0)
> +        if (!(name = strndup(p, t - p)))
>              goto no_memory;
>  
> -        if (!(machine->name = strndup(p, t - p))) {
> -            VIR_FREE(machine);
> -            goto no_memory;
> -        }
> -
> -        if (VIR_REALLOC_N(list, nitems + 1) < 0) {
> -            VIR_FREE(machine->name);
> -            VIR_FREE(machine);
> -            goto no_memory;
> -        }
> -
> -        p = t;
> -        if (!(t = strstr(p, "(default)")) || (next && t >= next)) {
> -            list[nitems++] = machine;
> -        } else {
> -            /* put the default first in the list */
> -            memmove(list + 1, list, sizeof(*list) * nitems);
> -            list[0] = machine;
> -            nitems++;
> -        }
> +        if (strstr(p, "(default)"))
> +            defIdx = caps->nmachineTypes;

While this will work because it sets defIdx to 1, 2, 4, ... until it stops at
the one which is really default. Can we preserve the condition that checks if
(default) was found at the current line rather than just somewhere in the rest
of the qemu output to make this less magic? And I would even preserve the
p = t assignment above.

>  
>          if ((t = strstr(p, "(alias of ")) && (!next || t < next)) {
>              p = t + strlen("(alias of ");
>              if (!(t = strchr(p, ')')) || (next && t >= next))
>                  continue;
...
> @@ -531,75 +515,52 @@ qemuCapsParsePPCModels(const char *output,
>          if (*p == '\n')
>              continue;
>  
> -        if (retcpus) {
> -            unsigned int len;
> -
> -            if (VIR_REALLOC_N(cpus, count + 1) < 0) {
> -                virReportOOMError();
> -                goto cleanup;
> -            }
> +        if (VIR_EXPAND_N(caps->cpuDefinitions, caps->ncpuDefinitions, 1) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
>  
> -            len = t - p - 1;
> +        len = t - p - 1;
>  
> -            if (!(cpus[count] = strndup(p, len))) {
> -                virReportOOMError();
> -                goto cleanup;
> -            }
> +        if (!(caps->cpuDefinitions[caps->ncpuDefinitions - 1] = strndup(p, len))) {
> +            virReportOOMError();
> +            goto cleanup;
>          }
> -        count++;
>      } while ((p = next));
>  
> -    if (retcount)
> -        *retcount = count;
> -    if (retcpus) {
> -        *retcpus = cpus;
> -        cpus = NULL;
> -    }
>      ret = 0;
>  
>  cleanup:
> -    if (cpus) {
> -        for (i = 0; i < count; i++)
> -            VIR_FREE(cpus[i]);
> -        VIR_FREE(cpus);
> -    }
>      return ret;
>  }
>  
> -int
> -qemuCapsProbeCPUModels(const char *qemu,
> -                       qemuCapsPtr caps,
> -                       const char *arch,
> -                       size_t *count,
> -                       const char ***cpus)
> +static int
> +qemuCapsProbeCPUModels(qemuCapsPtr caps)
>  {
>      char *output = NULL;
>      int ret = -1;
>      qemuCapsParseCPUModels parse;
>      virCommandPtr cmd;
>  
> -    if (count)
> -        *count = 0;
> -    if (cpus)
> -        *cpus = NULL;
> -
> -    if (STREQ(arch, "i686") || STREQ(arch, "x86_64"))
> +    if (STREQ(caps->arch, "i686") ||
> +        STREQ(caps->arch, "x86_64"))
>          parse = qemuCapsParseX86Models;
> -    else if (STREQ(arch, "ppc64"))
> +    else if (STREQ(caps->arch, "ppc64"))
>          parse = qemuCapsParsePPCModels;
>      else {
> -        VIR_DEBUG("don't know how to parse %s CPU models", arch);
> +        VIR_DEBUG("don't know how to parse %s CPU models",
> +                  caps->arch);
>          return 0;
>      }

It's already been there but we changed coding guidelines to all or nothing for
{} usage in if statements. Thus, while you are changing this, you can add {}
as well.

>  
> -    cmd = qemuCapsProbeCommand(qemu, caps);
> +    cmd = qemuCapsProbeCommand(caps->binary, caps);
>      virCommandAddArgList(cmd, "-cpu", "?", NULL);
>      virCommandSetOutputBuffer(cmd, &output);
>  
>      if (virCommandRun(cmd, NULL) < 0)
>          goto cleanup;
>  
> -    if (parse(output, count, cpus) < 0)
> +    if (parse(output, caps) < 0)
>          goto cleanup;
>  
>      ret = 0;
...

ACK with the nits fixed.

Jirka