On Mon, 2009-07-27 at 15:36 +0200, Daniel Veillard wrote:
On Thu, Jul 23, 2009 at 06:34:41PM +0100, Mark McLoughlin wrote:
A subsequent commit will add a "canonical" field to this structure, this patch basically just prepares the way for that.
The new type is added, along with virCapabilitiesAlloc/FreeMachines() helpers and a whole bunch of code to make the transition.
One quirk is that virCapabilitiesAddGuestDomain() and virCapabilitiesAddGuest() take ownership of the machine list rather than duping it. This makes sense to avoid needless copying.
* src/capabilities.h: add the virCapsGuestMachine struct and use it in virCapsGuestDomainInfo, add prototypes for new functions and update the AddGuest() prototypes
* src/capabilities.c: add code for allocating and freeing the new type, change the machines parameter to AddGuest() etc.
* src/libvirt_private.syms: export the new helpers
* src/qemu_conf.c: update all the machine type code to use the new struct
* src/xen_internal.c: ditto
* tests/testutilsqemu.c: ditto [...] +virCapsGuestMachinePtr * +virCapabilitiesAllocMachines(const char *const *names, int nnames) +{ + virCapsGuestMachinePtr *machines; + int i; + + if (VIR_ALLOC_N(machines, nnames) < 0) + return NULL; + + for (i = 0; i < nnames; i++) { + if (VIR_ALLOC(machines[i]) < 0 || + !(machines[i]->name = strdup(names[i]))) {
we should emit an OOM error here
Caller should emit the OOM error if we return NULL, I think that's common for allocators.
@@ -296,10 +296,16 @@ qemudParseMachineTypesStr(const char *output, if (!(t = strchr(p, ' ')) || (next && t >= next)) continue;
- if (!(machine = strndup(p, t - p))) + if (VIR_ALLOC(machine) < 0) goto error;
+ if (!(machine->name = strndup(p, t - p))) { + VIR_FREE(machine); + goto error; + } + if (VIR_REALLOC_N(list, nitems + 1) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); goto error; }
Same here the strndup failure paths should emit OOM errors
@@ -321,15 +327,13 @@ qemudParseMachineTypesStr(const char *output, return 0;
error: - for (i = 0; i < nitems; i++) - VIR_FREE(list[i]); - VIR_FREE(list); + virCapabilitiesFreeMachines(list, nitems); return -1; }
maybe a no_memory: label with the call would be the simplest, we use that in other places.
Caller is supposed to set error on OOM, I've fixed qemudCanonicalizeMachineDirect() to do that See below - it's the caller of qemudCapsInit() which eventually sets the error
@@ -434,12 +438,18 @@ qemudCapsInitGuest(virCapsPtr caps, return 0;
if (info->machine) { - char *machine; + virCapsGuestMachinePtr machine; + + if (VIR_ALLOC(machine) < 0) + return -1;
- if (!(machine = strdup(info->machine))) + if (!(machine->name = strdup(info->machine))) { + VIR_FREE(machine); return -1; + }
if (VIR_ALLOC_N(machines, nmachines) < 0) { + VIR_FREE(machine->name); VIR_FREE(machine); return -1; }
similar
There's lots of other cases in this function we don't set an error on OOM because the caller eventually sets an error: if ((qemu_driver->caps = qemudCapsInit(NULL)) == NULL) goto out_of_memory; Cheers, Mark.