[libvirt] [PATCH 20/21] tests: qemuxml2argv: move DO_CAPS_TEST* qemuCaps init

[Andrea Bolognani]

On Thu, 2019-03-14 at 10:44 -0400, Cole Robinson wrote:
[...]
> @@ -612,20 +614,25 @@ typedef enum {
>      ARG_MIGRATE_FD,
>      ARG_FLAGS,
>      ARG_PARSEFLAGS,
> +    ARG_CAPS_ARCH,
> +    ARG_CAPS_VER,

I guess these could be simply ARG_ARCH and ARG_VER, but I don't
have a terribly strong opinion on the subject.

[...]
> +testInfoSetArgs(struct testInfo *info, virHashTablePtr capslatest, ...)

One argument per line, please.

>  {
>      va_list argptr;
>      testInfoArgNames argname;
>      virQEMUCapsPtr qemuCaps = NULL;
>      int gic = GIC_NONE;
>      int ret = -1;
> +    char *capsarch = NULL;
> +    char *capsver = NULL;
> +    VIR_AUTOFREE(char *) capsfile = NULL;

ret should be the last variable in the list.

[...]
> +    if (!qemuCaps && capsarch && capsver) {
> +        bool stripmachinealiases = false;

I think it would be a good idea to perform some more validation on
the input here: for example we should error out if ARG_CAPS_ARCH has
been provided but ARG_CAPS_VER hasn't or viceversa, or if both
ARG_QEMU_CAPS and ARG_CAPS_ARCH+ARG_CAPS_VER have been provided,
because in both scenarios the user is probably not getting whatever
result they were expecting.

> +        if (STREQ(capsver, "latest")) {
> +            if (VIR_STRDUP(capsfile, virHashLookup(capslatest, capsarch)) < 0)
> +                goto cleanup;
> +            stripmachinealiases = true;
> +        } else if (virAsprintf(&capsfile,
> +                               TEST_CAPS_PATH "%s.%s.xml",
> +                               capsver, capsarch) < 0) {

I'd prefer this as

  virAsprintf(&capsfile, "%s/caps_%s.%s.xml",
              TEST_CAPS_PATH, capsver, capsarch)

with TEST_CAPS_PATH being redefined as

  abs_srcdir "/qemucapabilitiesdata"

of course: according to the name, it's supposed to be a path and
not a prefix after all.


Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization