[libvirt] [PATCH 4/5] viralloc: Introduce OOM error reporting to VIR_ALLOC and friends

[Eric Blake]

On 04/02/2013 08:22 AM, Michal Privoznik wrote:
> ---
>  HACKING                     |   4 ++
>  docs/hacking.html.in        |   4 ++
>  po/POTFILES.in              |   1 +
>  python/libvirt-override.c   | 102 ++++++++++++++++++++++----------------------
>  src/esx/esx_vi.c            |   5 +--
>  src/util/viralloc.c         |  91 +++++++++++++++++++++++++++++++++------
>  src/util/viralloc.h         |  52 +++++++++++++++-------
>  src/util/virbuffer.c        |   8 ++--
>  src/util/virerror.c         |   4 +-
>  src/util/virthreadpthread.c |   2 +-
>  tests/commandhelper.c       |   2 +-
>  tests/networkxml2conftest.c |   2 +-
>  tests/openvzutilstest.c     |   2 +-
>  tests/test_conf.c           |   2 +-
>  tests/xmconfigtest.c        |   2 +-
>  15 files changed, 188 insertions(+), 95 deletions(-)
> 
> diff --git a/HACKING b/HACKING
> index 6230ffd..c511cab 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -595,6 +595,10 @@ size:
>  
>  
>  
> +These memory allocation macros report OOM error automatically. In case, you
> +want to suppress such behavior, use their NOOOM variant, e.g. VIR_ALLOCNOOM,
> +VIR_REALLOC_NNOOM etc.

Three different spellings in one sentence!  NOOOM, NOOM, NNOOM.  All the
more reason to consider my 3/5 suggestion of a simpler name, such as
VIR_ALLOC_QUIET.

> -    if (VIR_ALLOC_N(ret, size) < 0) {
> +    if (VIR_ALLOC_NNOOM(ret, size) < 0) {

Oh, I misread your spellings.  If anything, have an underscore as part
of the suffix:

VIR_ALLOC_N_NOOM (or VIR_ALLOC_N_QUIET)

so that it is obvious that N (a count to alloc) and NOOM (or QUIET) is
an attribute of what to do on allocation failure, instead of wondering
if NNOOM is a typo.

> +++ b/src/esx/esx_vi.c
> @@ -1769,10 +1769,9 @@ esxVI_Alloc(void **ptrptr, size_t size)
>          return -1;
>      }
>  
> -    if (virAllocN(ptrptr, size, 1) < 0) {
> -        virReportOOMError();
> +    if (virAllocN(ptrptr, size, 1, true, VIR_FROM_THIS,
> +                  __FILE__, __FUNCTION__, __LINE__) < 0)
>          return -1;
> -    }

Can we rework this to use VIR_ALLOC_N_NOOM(*ptrptr, size 1)? instead of
the raw call to virAllocN, along with the cleanup to cfg.mk?

> @@ -270,7 +316,8 @@ int virResizeN(void *ptrptr, size_t size, size_t *allocptr, size_t count,
>  void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove)
>  {
>      if (toremove < *countptr)
> -        ignore_value(virReallocN(ptrptr, size, *countptr -= toremove));
> +        ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, false,
> +                                 0, NULL, NULL, 0));

How come you aren't adding location arguments to virResizeN and touching
up VIR_RESIZE_N to pass in location?

In fact, it may be worth splitting this into two patches - one that adds
location information to existing functions, and the second that
introduces new functions and adds the 'report' option.

> @@ -318,7 +365,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
>  
>      if (inPlace) {
>          *countptr += add;
> -    } else if (virExpandN(ptrptr, size, countptr, add) < 0) {
> +    } else if (virExpandN(ptrptr, size, countptr, add,
> +                          false, 0, NULL, NULL, 0) < 0) {
>          return -1;

Another function where adding location may make sense.

> @@ -82,7 +90,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1);
>   *
>   * Returns -1 on failure, 0 on success
>   */
> -# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
> +# define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)), true, VIR_FROM_THIS, \
> +                                 __FILE__, __FUNCTION__, __LINE__)
> +# define VIR_ALLOCNOOM(ptr) virAlloc(&(ptr), sizeof(*(ptr)), false, 0, NULL, NULL, 0)

Hmm.  At first, I wondered if still passing VIR_FROM_THIS, __FILE__,
__FUNCTION__, __LINE__ on through to virAlloc, even if they aren't used,
would aid in debugging.  But then I realized that if you can stop the
debugger on virAlloc failure, then you can also use the debugger to
determine the callstack.  So this seems fine, after all (for the
trailing arguments - the choice of naming is still an open issue...).

> +++ b/tests/openvzutilstest.c
> @@ -101,7 +101,7 @@ testReadNetworkConf(const void *data ATTRIBUTE_UNUSED)
>          "  </devices>\n"
>          "</domain>\n";
>  
> -    if (VIR_ALLOC(def) < 0 ||
> +    if (VIR_ALLOCNOOM(def) < 0 ||
>          !(def->os.type = strdup("exe")) ||
>          !(def->os.init = strdup("/sbin/init")))

I thought 2/5 got rid of all raw strdup()?  Or maybe the syntax check
rule exempted a bit too much?  Again an argument for making things do a
per-use exemption, instead of a per-file exemption, the way we do for
strtol.

The places where you used the new function look sane.  Looking forward
to v2 with saner names.  Again, splitting into two patches (one to make
viralloc.h preserve locations but with no new functionality, and another
to add the new functionality) might make review easier.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130402/d00a1ddd/attachment-0001.sig>