Re: [libvirt] [PATCH 20/21] tests: qemuxml2argv: move DO_CAPS_TEST* qemuCaps init
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