[libvirt] [PATCH 2/7] util: make allocation functions abort on OOM

[Daniel P. Berrangé]

On Thu, Sep 05, 2019 at 06:03:57PM -0400, John Ferlan wrote:
> 
> 
> On 8/29/19 2:02 PM, Daniel P. Berrangé wrote:
> > The functions are left returning an "int" to avoid an immediate
> > big-bang cleanup. They'll simply never return anything other
> > than 0, except for virInsertN which can still return an error
> > if the requested insertion index is out of range. Interestingly
> > in that case, the _QUIET function would none the less report
> > an error.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> > ---
> >  src/util/viralloc.c | 201 +++++++++++---------------------------------
> >  src/util/viralloc.h | 145 +++++++++++---------------------
> >  2 files changed, 97 insertions(+), 249 deletions(-)
> > 
> 
> 
> FWIW: I applied the series to my Coverity branch to see what (if
> anything) gets shaken there.  Good news is - not much. There were a
> couple of things I already reported on that were fixed for 5.7.0 (commit
> ed7e342b0 and be9d259eb).
> 
> There's one more false positive in qemuDomainGetNumaParameters that
> probably would be fixed by AUTOFREE stuff (nodeset gets set to NULL
> after virTypedParameterAssign).
> 
> Beyond that the only thing is two CHECKED_RETURN's (e.g. N callers check
> the return value, but these 2 don't).  I consider both false positives;
> however, it could also be argued that unless any of the functions where
> either 0 or abort occurs, then make them just void, but that's a sh*load
> more work...
> 
> [...]
> 
> >  /**
> > @@ -204,50 +143,35 @@ int virExpandN(void *ptrptr,
> >   * @allocptr: pointer to number of elements allocated in array
> >   * @count: number of elements currently used in array
> >   * @add: minimum number of additional elements to support in array
> > - * @report: whether to report OOM error, if there is one
> > - * @domcode: error domain code
> > - * @filename: caller's filename
> > - * @funcname: caller's funcname
> > - * @linenr: caller's line number
> >   *
> >   * If 'count' + 'add' is larger than '*allocptr', then resize the
> >   * block of memory in 'ptrptr' to be an array of at least 'count' +
> >   * 'add' elements, each 'size' bytes in length. Update 'ptrptr' and
> >   * 'allocptr' with the details of the newly allocated memory. On
> >   * failure, 'ptrptr' and 'allocptr' are not changed. Any newly
> > - * allocated memory in 'ptrptr' is zero-filled. If @report is true,
> > - * OOM errors are reported automatically.
> > - *
> > + * allocated memory in 'ptrptr' is zero-filled.
> >   *
> > - * Returns -1 on failure to allocate, zero on success
> > + * Returns zero on success, aborts on OOM
> >   */
> >  int virResizeN(void *ptrptr,
> >                 size_t size,
> >                 size_t *allocptr,
> >                 size_t count,
> > -               size_t add,
> > -               bool report,
> > -               int domcode,
> > -               const char *filename,
> > -               const char *funcname,
> > -               size_t linenr)
> > +               size_t add)
> >  {
> >      size_t delta;
> >  
> > -    if (count + add < count) {
> > -        if (report)
> > -            virReportOOMErrorFull(domcode, filename, funcname, linenr);
> > -        errno = ENOMEM;
> > -        return -1;
> > -    }
> > +    if (count + add < count)
> > +        abort();
> > +
> >      if (count + add <= *allocptr)
> >          return 0;
> >  
> >      delta = count + add - *allocptr;
> >      if (delta < *allocptr / 2)
> >          delta = *allocptr / 2;
> > -    return virExpandN(ptrptr, size, allocptr, delta, report,
> > -                      domcode, filename, funcname, linenr);
> > +    virExpandN(ptrptr, size, allocptr, delta);
> > +    return 0;
> 
> Could just return virExpandN here...
> 
> >  }
> 
> [...]
> 
> > @@ -312,12 +229,7 @@ int
> >  virInsertElementsN(void *ptrptr, size_t size, size_t at,
> >                     size_t *countptr,
> >                     size_t add, void *newelems,
> > -                   bool clearOriginal, bool inPlace,
> > -                   bool report,
> > -                   int domcode,
> > -                   const char *filename,
> > -                   const char *funcname,
> > -                   size_t linenr)
> > +                   bool clearOriginal, bool inPlace)
> >  {
> >      if (at == -1) {
> >          at = *countptr;
> > @@ -330,9 +242,8 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at,
> >  
> >      if (inPlace) {
> >          *countptr += add;
> > -    } else if (virExpandN(ptrptr, size, countptr, add, report,
> > -                          domcode, filename, funcname, linenr) < 0) {
> > -        return -1;
> > +    } else {
> > +        virExpandN(ptrptr, size, countptr, add);
> >      }
> >  
> 
> This one's more painful, with using ignore_value() wrapper to just
> pacify Coverity.
> 
> [...]
> 
> I'm not saying anything has to be done, but I figured it could be a
> useful data point for you -

In this patch I removed the ATTRIBUTE_RETURN_CHECK annotation, but I'm
going to revert that bit.

We don't really want churn from

   if (VIR_ALLOC(foo) < 0)
      return -1;

to

   VIR_ALLOC(foo);

to

   foo = g_new0(struct Foo, 1)

By keeping ATTRIBUTE_RETURN_CHECK we forcably avoid the intermediate
conversion step, which will keep things saner I think. That will
avoid creating 100's of new coverity warnings for the intermediate
step.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|