Re: [libvirt] [PATCH] Fix misc OOM bugs

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" ?

> 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.