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

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(a)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(a)redhat.com&gt; (series) John