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

[Cole Robinson]

On 3/19/19 11:18 AM, Andrea Bolognani wrote:
> 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.
> 

I figured ARG_ARCH/ARG_VER are ambiguous enough, the CAPS was to make
it clear this was specifically for caps file lookups

> [...]
>> +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.
> 

I'll handle this in a separate patch

>> +        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.
>

Sure, I'll break that change out into a prep patch. v2 incoming

Thanks,
Cole