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@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@redhat.com> (series) John