On Thu, 2018-06-14 at 22:32 +0200, Lubomir Rintel wrote:
> +static int testQemuAddRISCV32Guest(virCapsPtr caps)
> +{
> + static const char *machine[] = { "spike_v1.10",
> + "spike_v1.9.1",
> + "sifive_e",
> + "virt",
> + "sifive_u" };
> + virCapsGuestMachinePtr *machines = NULL;
> + virCapsGuestPtr guest;
> +
> + machines = virCapabilitiesAllocMachines(machine, 1);
This is wrong: the second argument to the function is the number
of machines you're adding, so it should look like
virCapabilitiesAllocMachines(machine,
ARRAY_CARDINALITY(machine))
instead. While you're at it, you can give the variable a better
name, like for example... 'names' :)
Actually, since you're going to need that value more than once,
you will probably want to introduce
static const int nmachines = ARRAY_CARDINALITY(names);
and just use 'nmachines' in the call to AllocMachines() above...
> + if (!machines)
> + goto error;
> +
> + guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_RISCV32,
> + QEMUBinList[TEST_UTILS_QEMU_BIN_RISCV32],
> + NULL, 1, machines);
... as well as here...
> + if (!guest)
> + goto error;
> +
> + if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, NULL, 0, NULL))
> + goto error;
> +
> + return 0;
> +
> + error:
> + /* No way to free a guest? */
> + virCapabilitiesFreeMachines(machines, 1);
... and here.
(Drop the comment here as well.)
Looking at this also made me realize there are several existing
instances of it not being handled correctly; I'll take care of
fixing them.
[...]
> @@ -422,6 +488,12 @@ virCapsPtr testQemuCapsInit(void)
> if (testQemuAddPPCGuest(caps))
> goto cleanup;
>
> + if (testQemuAddRISCV32Guest(caps))
> + goto cleanup;
> +
> + if (testQemuAddRISCV64Guest(caps))
> + goto cleanup;
Despite the surrounding code suggesting otherwise, both these calls
should look like
if (testQemuAddRISCV32Guest(caps) < 0)
goto cleanup;
Again, I'll take care of fixing the existing instances.
Once all of the above have been taken care of, the patch will look
good; however, I think it doesn't make a lot of sense to merge it
on its own, and we should rather squash it into 07/11 instead and
use a commit message like
tests: Add RISC-V architectures
for the whole thing.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
