[libvirt] [PATCH 0/4] Make it easier to clean up after using virBuffer

[John Ferlan]

On 08/21/2017 03:47 AM, Martin Kletzander wrote:
> There are many places in the code where virBufferCheckError() is used
> and then, right after that, virBufferContentAndReset() is called.  The
> former has ATTRIBUTE_RETURN_CHECK, so every occurrence just checks
> that.  However, if the return value of the latter is also the return
> value of the current function, there is no need to duplicate the work
> and act upon the error twice.

If code doesn't really care or need to check the error, then it's
unnecessary...

> 
> This series proposes the idea of virCheckError being used for only
> reporting the error [1/4] and shows an example commit on how to clean
> up existing functions [2/4] so that it can be posted to our wiki under
> https://wiki.libvirt.org/page/BiteSizedTasks and linked from there.>
> Further enhancements could go a step further and create one function
> (actually a macro the same way CheckError is done) and wrap those two
> lines in one so that it is even shorter.  This, however, is not meant
> to be part of this series.
> 
> Patches [3/4] and [4/4] utilize this for miscellaneous clean-ups in
> src/conf/.
> 
> 
> Martin Kletzander (4):
>   util: Umark virBufferCheckErrorInternal as ATTRIBUTE_RETURN_CHECK

Consider $subj as:

util: Remove ATTRIBUTE_RETURN_CHECK from virBufferCheckErrorInternal

but see my Coverity note below first...

>   util: Use virBufferCheckError to its full potential.
>   conf: Clean up and report error in virDomainCapsFormat
>   conf: Clean up and report error in virDomainGenerateMachineName
> 
>  src/conf/domain_capabilities.c | 68 +++++++++++++++++-------------------------
>  src/conf/domain_conf.c         | 16 +++++-----
>  src/util/virbitmap.c           |  6 +---
>  src/util/virbuffer.h           |  2 +-
>  4 files changed, 37 insertions(+), 55 deletions(-)
> 
> --
> 2.14.1
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

While what you have works just fine - it causes new Coverity warnings
regarding the fact that 209 of 212 places check for error, but the 3 new
callers don't.

As an alternative, why not add and use:

/**
 * virBufferReportError
 *
 * Similar to above, but wraps inside an ignore_value for paths that
 * don't need to check the return status.
 */
# define virBufferReportError(buf) \
    ignore_value(virBufferCheckErrorInternal(buf, VIR_FROM_THIS, \
    __FILE__, __FUNCTION__, __LINE__))

Instead of dropping the ATTRIBUTE_RETURN_CHECK

IDC either way - less places for me to add ignore_value() wrapper ;-)

Whichever way you decide is fine by me though, so

Reviewed-by: John Ferlan <jferlan at redhat.com>  (series)

John