Re: [libvirt] [PATCH v2 08/11] tests/util: add RISC-V capabilities
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