2:47 a.m.
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.
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
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
2:47 a.m.
New subject: [libvirt] [PATCH 1/4] util: Umark virBufferCheckErrorInternal as ATTRIBUTE_RETURN_CHECK
The function is useful even without using the return value. And if
needed, the return value can be obtained by other calls as well. The
potential for clean-up can be seen in the following patch.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virbuffer.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index f34217968b83..248c135b6077 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -58,7 +58,7 @@ int virBufferCheckErrorInternal(const virBuffer *buf,
const char *filename,
const char *funcname,
size_t linenr)
- ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1);
+ ATTRIBUTE_NONNULL(1);
/**
* virBufferCheckError
*
--
2.14.1
2:47 a.m.
New subject: [libvirt] [PATCH 2/4] util: Use virBufferCheckError to its full potential.
We can now check for the error and not care about the return value as
it will be properly handled in virBufferContentAndReset() anyway.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/util/virbitmap.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index a5077fe7c8bf..57a8fbf82c78 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -332,11 +332,7 @@ char *virBitmapString(virBitmapPtr bitmap)
bitmap->map[sz]);
}
- if (virBufferError(&buf)) {
- virBufferFreeAndReset(&buf);
- return NULL;
- }
-
+ virBufferCheckError(&buf);
return virBufferContentAndReset(&buf);
}
--
2.14.1
2:47 a.m.
New subject: [libvirt] [PATCH 3/4] conf: Clean up and report error in virDomainCapsFormat
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/conf/domain_capabilities.c | 68 +++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 40 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 7a81c10fd10f..35f8128e70dd 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -516,62 +516,50 @@ virDomainCapsFeatureGICFormat(virBufferPtr buf,
}
-static int
-virDomainCapsFormatInternal(virBufferPtr buf,
- virDomainCapsPtr const caps)
+char *
+virDomainCapsFormat(virDomainCapsPtr const caps)
{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
const char *virttype_str = virDomainVirtTypeToString(caps->virttype);
const char *arch_str = virArchToString(caps->arch);
- virBufferAddLit(buf, "<domainCapabilities>\n");
- virBufferAdjustIndent(buf, 2);
+ virBufferAddLit(&buf, "<domainCapabilities>\n");
+ virBufferAdjustIndent(&buf, 2);
- virBufferEscapeString(buf, "<path>%s</path>\n",
caps->path);
- virBufferAsprintf(buf, "<domain>%s</domain>\n", virttype_str);
+ virBufferEscapeString(&buf, "<path>%s</path>\n",
caps->path);
+ virBufferAsprintf(&buf, "<domain>%s</domain>\n",
virttype_str);
if (caps->machine)
- virBufferAsprintf(buf, "<machine>%s</machine>\n",
caps->machine);
- virBufferAsprintf(buf, "<arch>%s</arch>\n", arch_str);
+ virBufferAsprintf(&buf, "<machine>%s</machine>\n",
caps->machine);
+ virBufferAsprintf(&buf, "<arch>%s</arch>\n", arch_str);
if (caps->maxvcpus)
- virBufferAsprintf(buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
+ virBufferAsprintf(&buf, "<vcpu max='%d'/>\n",
caps->maxvcpus);
- virDomainCapsOSFormat(buf, &caps->os);
- virDomainCapsCPUFormat(buf, &caps->cpu);
+ virDomainCapsOSFormat(&buf, &caps->os);
+ virDomainCapsCPUFormat(&buf, &caps->cpu);
- virBufferAddLit(buf, "<devices>\n");
- virBufferAdjustIndent(buf, 2);
+ virBufferAddLit(&buf, "<devices>\n");
+ virBufferAdjustIndent(&buf, 2);
- virDomainCapsDeviceDiskFormat(buf, &caps->disk);
- virDomainCapsDeviceGraphicsFormat(buf, &caps->graphics);
- virDomainCapsDeviceVideoFormat(buf, &caps->video);
- virDomainCapsDeviceHostdevFormat(buf, &caps->hostdev);
+ virDomainCapsDeviceDiskFormat(&buf, &caps->disk);
+ virDomainCapsDeviceGraphicsFormat(&buf, &caps->graphics);
+ virDomainCapsDeviceVideoFormat(&buf, &caps->video);
+ virDomainCapsDeviceHostdevFormat(&buf, &caps->hostdev);
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</devices>\n");
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</devices>\n");
- virBufferAddLit(buf, "<features>\n");
- virBufferAdjustIndent(buf, 2);
+ virBufferAddLit(&buf, "<features>\n");
+ virBufferAdjustIndent(&buf, 2);
- virDomainCapsFeatureGICFormat(buf, &caps->gic);
+ virDomainCapsFeatureGICFormat(&buf, &caps->gic);
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</features>\n");
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</features>\n");
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</domainCapabilities>\n");
- return 0;
-}
-
-
-char *
-virDomainCapsFormat(virDomainCapsPtr const caps)
-{
- virBuffer buf = VIR_BUFFER_INITIALIZER;
-
- if (virDomainCapsFormatInternal(&buf, caps) < 0) {
- virBufferFreeAndReset(&buf);
- return NULL;
- }
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</domainCapabilities>\n");
+ virBufferCheckError(&buf);
return virBufferContentAndReset(&buf);
}
--
2.14.1
2:47 a.m.
New subject: [libvirt] [PATCH 4/4] conf: Clean up and report error in virDomainGenerateMachineName
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
src/conf/domain_conf.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3bef5bed3b4a..fffdeb353581 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27151,25 +27151,23 @@ virDomainGenerateMachineName(const char *drivername,
const char *name,
bool privileged)
{
- char *machinename = NULL;
char *username = NULL;
virBuffer buf = VIR_BUFFER_INITIALIZER;
if (privileged) {
virBufferAsprintf(&buf, "%s-", drivername);
} else {
- if (!(username = virGetUserName(geteuid())))
- goto cleanup;
-
+ if (!(username = virGetUserName(geteuid()))) {
+ virBufferFreeAndReset(&buf);
+ return NULL;
+ }
virBufferAsprintf(&buf, "%s-%s-", username, drivername);
+ VIR_FREE(username);
}
virBufferAsprintf(&buf, "%d-", id);
virDomainMachineNameAppendValid(&buf, name);
- machinename = virBufferContentAndReset(&buf);
- cleanup:
- VIR_FREE(username);
-
- return machinename;
+ virBufferCheckError(&buf);
+ return virBufferContentAndReset(&buf);
}
--
2.14.1
11:56 a.m.
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> (series)
John
1:19 a.m.
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(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.
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(a)redhat.com> (series)
John
Thanks, I'll wait for your further comments.
6:43 a.m.
On 08/25/2017 02:19 AM, Martin Kletzander wrote:
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(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.
>
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?
I don't know exactly how Coverity counts and keeps track because there
are some that don't show up at all and then one day someone pushes a
change and a warning is issue that N of M check status where M is not
necessarily N+1. Sometimes it's N+5 or N+8. It seems as though there's
heuristics that know when we go from 100% checking status to some value
less as much as it knows that there were (for example) 90% of the calls
checking and some change pushed that to 95% and now flags those other
5%. That to me is the oddest of the cases, but is one I've seen happen.
That is someone made a change to foo.c and then Coverity messages on
bar.c that's using the same function without checks.
So that all says to me perhaps it's a false positive type message and it
can always be ignored; however, that isn't always the case. So, I think
the purpose of the message is to ensure that the places that are (now)
failing the test are checked to ensure nothing's being missed such as no
status check on some function that ends up allocating memory. The goal
being to cause the user of coverity to check just in case.
> 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
Well if the "long term plan" is to make return value checking
unnecessary for some virBufferCheckErrorInternal, then there's a couple
of options. I presented one alternative which seemed simple enough.
I know altering changes for what is perceived as a false positive in
coverity is not generally a desired option, so I'm fine with the code as
is since when this human reads the code he can easily determine the
virBufferCheckErrorInternal doesn't need to have it's return value
checked since virBufferContentAndReset will handle the error properly.
Your cover letter indicated future enhancements in order to cover some
of the other cases which is what got me thinking perhaps this could
change so that we don't end up with those future enhancements running
into the same issue (assuming anyone picks up the task).
So, if I didn't mention that it was coverity complaining and instead
said - why not take the plunge now and create a void function that does
the same thing as the int function, would that have made a difference?
I'd rather see a #define wrapper that allows one to ignore the status
because they don't care. That leaves it to the caller to decide whether
or not to check the returned status. I just chose to explain why.
> 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.
It won't be the first time... I've given up on others - I just add them
to my local branch. For example, qemuBuildShmemBackendMemProps calls
virJSONValueObjectCreate which in all other cases expects status return
check; however, for this function it doesn't make sense since the caller
would be checking the returned @ret anyway. Still Coverity complains, so
in order to avoid that I wrapped the call in ignore_value. If someone
makes change to the same place my branch has a merge conflict which I
have to resolve. There's similar type workarounds in my local branch for
qemuMigrationCookieStatisticsXMLParse and the *last* call to virXPathInt
as well as qemuCgroupEmulatorAllNodesRestore and the call to
virCgroupSetCpusetMems.
I currently have 30 patches of varying degrees of false positives.
Things that have been NACK'd in upstream reviews as a waste of a patch
to work around what some consider a defect in the analyzer. I have a
fair number of /* coverity[leaked_storage] */ because functions like
virJSONValueObjectCreate will "consume" the address of something via the
"a:XXXX" argument, but coverity cannot determine that so it just
elicits the warning to ensure the consumer checks to be sure.
John
> Reviewed-by: John Ferlan <jferlan(a)redhat.com> (series)
>
> John
Thanks, I'll wait for your further comments.
7:14 a.m.
On Fri, Aug 25, 2017 at 07:43:44AM -0400, John Ferlan wrote:
On 08/25/2017 02:19 AM, Martin Kletzander wrote:
> 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(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.
>>
>
> 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?
>
I don't know exactly how Coverity counts and keeps track because there
are some that don't show up at all and then one day someone pushes a
change and a warning is issue that N of M check status where M is not
necessarily N+1. Sometimes it's N+5 or N+8. It seems as though there's
heuristics that know when we go from 100% checking status to some value
less as much as it knows that there were (for example) 90% of the calls
checking and some change pushed that to 95% and now flags those other
5%. That to me is the oddest of the cases, but is one I've seen happen.
That is someone made a change to foo.c and then Coverity messages on
bar.c that's using the same function without checks.
So that all says to me perhaps it's a false positive type message and it
can always be ignored; however, that isn't always the case. So, I think
the purpose of the message is to ensure that the places that are (now)
failing the test are checked to ensure nothing's being missed such as no
status check on some function that ends up allocating memory. The goal
being to cause the user of coverity to check just in case.
Oh, interesting to know. I kind of get the reasoning behind
this... Kind of.
>> 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
>
Well if the "long term plan" is to make return value checking
unnecessary for some virBufferCheckErrorInternal, then there's a couple
of options. I presented one alternative which seemed simple enough.
I know altering changes for what is perceived as a false positive in
coverity is not generally a desired option, so I'm fine with the code as
is since when this human reads the code he can easily determine the
virBufferCheckErrorInternal doesn't need to have it's return value
checked since virBufferContentAndReset will handle the error properly.
Your cover letter indicated future enhancements in order to cover some
of the other cases which is what got me thinking perhaps this could
change so that we don't end up with those future enhancements running
into the same issue (assuming anyone picks up the task).
So, if I didn't mention that it was coverity complaining and instead
said - why not take the plunge now and create a void function that does
the same thing as the int function, would that have made a difference?
I'd rather see a #define wrapper that allows one to ignore the status
because they don't care. That leaves it to the caller to decide whether
or not to check the returned status. I just chose to explain why.
Sure, I'm generally fine with both, I just think one of them is cleaner.
>> 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.
>
It won't be the first time... I've given up on others - I just add them
to my local branch. For example, qemuBuildShmemBackendMemProps calls
virJSONValueObjectCreate which in all other cases expects status return
check; however, for this function it doesn't make sense since the caller
would be checking the returned @ret anyway. Still Coverity complains, so
in order to avoid that I wrapped the call in ignore_value. If someone
makes change to the same place my branch has a merge conflict which I
have to resolve. There's similar type workarounds in my local branch for
qemuMigrationCookieStatisticsXMLParse and the *last* call to virXPathInt
as well as qemuCgroupEmulatorAllNodesRestore and the call to
virCgroupSetCpusetMems.
I currently have 30 patches of varying degrees of false positives.
Things that have been NACK'd in upstream reviews as a waste of a patch
to work around what some consider a defect in the analyzer. I have a
fair number of /* coverity[leaked_storage] */ because functions like
virJSONValueObjectCreate will "consume" the address of something via the
"a:XXXX" argument, but coverity cannot determine that so it just
elicits the warning to ensure the consumer checks to be sure.
I get your point, on the other hand I must say I agree with the reviews
most of the time. But please don't waste time on the patches. They
will only accumulate and eat up more and more of your time during
rebases. I'm sure the conflicts are fine, but still... My feeling is
that adding workarounds for coverity are sometimes fine (coverity can
find interesting stuff), but whenever it hinders the code readability it
inherently makes people do more mistakes.
I'm really sorry for this, but I really don't see the point, in this
particular case, for making coverity happy. I'll go with my approach,
but I promise you I'll be telling newbies about this task in particular.
Thanks for the review and thorough explanation of the process.
Martin
2646
days inactive
2650
days old
8 comments
2 participants
participants (2)
-
John Ferlan -
Martin Kletzander