[libvirt] [PATCH] Fix misc OOM bugs

[Daniel P. Berrange]

On Thu, Sep 03, 2009 at 12:07:33PM +0100, Mark McLoughlin wrote:
> On Wed, 2009-09-02 at 15:11 +0100, Daniel P. Berrange wrote:
> > * tests/testutils.c: Run test function twice, once to prime it for
> >   static allocations, once to count the non-static allocations.
> > * tests/testutilsqemu.c: Initialize variable correctl
> > * src/capabilities.c: Don't free machines variable upon failure
> >   since caller must do that
> > * src/xm_internal.c: Add missing check for OOM in building VIF
> >   config param
> > ---
> >  src/capabilities.c    |   17 +++++++++--------
> >  src/xm_internal.c     |    3 +++
> >  tests/testutils.c     |   12 ++++++++----
> >  tests/testutilsqemu.c |    2 +-
> >  4 files changed, 21 insertions(+), 13 deletions(-)
> > 
> > diff --git a/src/capabilities.c b/src/capabilities.c
> > index 193a9fe..289180d 100644
> > --- a/src/capabilities.c
> > +++ b/src/capabilities.c
> > @@ -354,10 +354,6 @@ virCapabilitiesAddGuest(virCapsPtr caps,
> >      if (loader &&
> >          (guest->arch.defaultInfo.loader = strdup(loader)) == NULL)
> >          goto no_memory;
> > -    if (nmachines) {
> > -        guest->arch.defaultInfo.nmachines = nmachines;
> > -        guest->arch.defaultInfo.machines = machines;
> > -    }
> >  
> >      if (VIR_REALLOC_N(caps->guests,
> >                        caps->nguests + 1) < 0)
> > @@ -365,6 +361,11 @@ virCapabilitiesAddGuest(virCapsPtr caps,
> >      caps->guests[caps->nguests] = guest;
> >      caps->nguests++;
> >  
> > +    if (nmachines) {
> > +        guest->arch.defaultInfo.nmachines = nmachines;
> > +        guest->arch.defaultInfo.machines = machines;
> > +    }
> > +
> >      return guest;
> >  
> >   no_memory:
> > @@ -407,10 +408,6 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest,
> >      if (loader &&
> >          (dom->info.loader = strdup(loader)) == NULL)
> >          goto no_memory;
> > -    if (nmachines) {
> > -        dom->info.nmachines = nmachines;
> > -        dom->info.machines = machines;
> > -    }
> >  
> >      if (VIR_REALLOC_N(guest->arch.domains,
> >                        guest->arch.ndomains + 1) < 0)
> > @@ -418,6 +415,10 @@ virCapabilitiesAddGuestDomain(virCapsGuestPtr guest,
> >      guest->arch.domains[guest->arch.ndomains] = dom;
> >      guest->arch.ndomains++;
> >  
> > +    if (nmachines) {
> > +        dom->info.nmachines = nmachines;
> > +        dom->info.machines = machines;
> > +    }
> 
> ACK to both, good catch.
> 
> > diff --git a/src/xm_internal.c b/src/xm_internal.c
> > index 71b852e..66f1e1c 100644
> > --- a/src/xm_internal.c
> > +++ b/src/xm_internal.c
> > @@ -2048,6 +2048,9 @@ static int xenXMDomainConfigFormatNet(virConnectPtr conn,
> >          virBufferVSprintf(&buf, ",vifname=%s",
> >                            net->ifname);
> >  
> > +    if (virBufferError(&buf))
> > +        goto cleanup;
> > +
> 
> ACK
> 
> Check usages of xenDaemonFormatSxprChr() for a similar error
> 
> > diff --git a/tests/testutils.c b/tests/testutils.c
> > index 7a1dbdc..5072fec 100644
> > --- a/tests/testutils.c
> > +++ b/tests/testutils.c
> > @@ -367,10 +367,7 @@ int virtTestMain(int argc,
> >          }
> >      }
> >  
> > -    if (testOOM)
> > -        virAllocTestInit();
> > -
> > -    /* Run once to count allocs, and ensure it passes :-) */
> > +    /* Run once to prime any static allocations & ensure it passes */
> >      ret = (func)(argc, argv);
> >      if (ret != EXIT_SUCCESS)
> >          goto cleanup;
> > @@ -385,6 +382,13 @@ int virtTestMain(int argc,
> >          testOOM++;
> >          virSetErrorFunc(NULL, virtTestErrorFuncQuiet);
> >  
> > +        virAllocTestInit();
> > +
> > +        /* Run again to count allocs, and ensure it passes :-) */
> > +        ret = (func)(argc, argv);
> > +        if (ret != EXIT_SUCCESS)
> > +            goto cleanup;
> > +
> 
> I don't follow this, maybe would be better as a separate patch with more
> explanation. What is "prime any static allocations" ?

The first time we run 'func', we count the number of memory allocations.
We then re-run 'func' that number of times and require that every single
invocation returns an error.  The problem was that in this first run
we were counting some one-time static allocations, so some of the 
subsequent runs would not in fact error. So we run once to prime
the static allocations, and again to get the true count of dynamic
allocations.

> 
> >          approxAlloc = virAllocTestCount();
> >          testCounter++;
> >          if (testDebug)
> > diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> > index 58707e1..6b6a185 100644
> > --- a/tests/testutilsqemu.c
> > +++ b/tests/testutilsqemu.c
> > @@ -9,7 +9,7 @@ virCapsPtr testQemuCapsInit(void) {
> >      struct utsname utsname;
> >      virCapsPtr caps;
> >      virCapsGuestPtr guest;
> > -    virCapsGuestMachinePtr *machines;
> > +    virCapsGuestMachinePtr *machines = NULL;
> 
> This doesn't look like it's needed.

Well the test suite was SEGV'ing on OOM without it, but perhaps a
subsequent change was removed the need for this I'll re-check



FYI, all these errors were automatically detected after building
with '--enable-test-oom' and running 

  VIR_TEST_OOM=1 make check

NB setting VIR_TEST_MP=$NUM-OF-CPUS  is a real benefit speedwise.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|