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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
