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

[Martin Kletzander]

On Wed, Aug 23, 2017 at 12:56:02PM -0400, John Ferlan wrote:
>
>
>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.
>

I don't really understand this warning.  If we have, let's say, 300
callers of memmove() and 290 of them use the return value, but 10 of
them don't, then it will issue a warning as well?

>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
>

I don't like the fact that it's workaround.  It's like having a function
that does four things at once and you call it and then revert two of
those things =D

>IDC either way - less places for me to add ignore_value() wrapper ;-)
>
>Whichever way you decide is fine by me though, so
>

Even if it generates coverity warnings?  I mean they are sometimes good
and can mean something (unless it's a thing that it cannot deduct from
the code, of course), but I don't understand this one.  If it adds ne
warnings, I'll go with your version, but I would rather understand it
first.

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

Thanks, I'll wait for your further comments.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170825/99e85a35/attachment-0001.sig>