Re: [libvirt] [PATCH 3/6] Add virCapsGuestMachine structure

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;