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

[Daniel Veillard]

On Mon, Jul 27, 2009 at 03:05:57PM +0100, Mark McLoughlin wrote:
> 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;

  Okay, thanks for the explanations, I lacked the context !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/