[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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

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

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


libvir-list mailing list
libvir-list redhat com

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

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


Thanks, I'll wait for your further comments.

Attachment: signature.asc
Description: Digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]