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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
