[libvirt] [PATCH] Fix misc OOM bugs

[Mark McLoughlin]

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.

Cheers,
Mark.