Re: [libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]
On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
> Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair
> to perform NULL argument checking; however, no corresponding change
> to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the
> Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
I didn't push yet...
The last time this happened is/was commit 425aac3abf.
Still, before this patch if you build with STATIC_ANALYSIS set you'd see
the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1
instead of 0, I get:
util/vircommand.c: In function 'virCommandAddArg':
util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL
[-Werror=nonnull-compare]
if (val == NULL) {
^
util/vircommand.c: In function 'virCommandAddArgPair':
util/vircommand.c:1604:14: error: nonnull argument 'name' compared to
NULL [-Werror=nonnull-compare]
if (name == NULL || val == NULL) {
^
util/vircommand.c:1604:29: error: nonnull argument 'val' compared to
NULL [-Werror=nonnull-compare]
if (name == NULL || val == NULL) {
^
Is there a different way you'd prefer this resolved? I could leave it as
a my coverity environment only, but I would assume clang would be
impacted too, although I don't use clang.
We have historically still added nonnull annotations even when
having checks for NULL in the impl. We had to explicitly disabble
the nonnull annotation when building under GCC to prevent it from
optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS
so that coverity would still report if any callers passed a NULL into
the methods.
That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less
important" because we found that it really only helps if someone calls
some API with NULL and it does not help if the argument itself is NULL.
My recollection is always towards removing it once the attribute itself
was checked in the API. I also have a faint recollection of a discussion
we've had before on this with the thought towards removing all the
ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but
that got to be too many patches and checks in too many places.
John
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> It's a build breaker for Coverity or anything that enables STATIC_ANALYSIS
> so I'll push under that rule.
>
> src/util/vircommand.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index dbf5041890..100f7a06e0 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -123,7 +123,7 @@ void virCommandAddEnvPassAllowSUID(virCommandPtr cmd,
> void virCommandAddEnvPassCommon(virCommandPtr cmd);
>
> void virCommandAddArg(virCommandPtr cmd,
> - const char *val) ATTRIBUTE_NONNULL(2);
> + const char *val);
>
> void virCommandAddArgBuffer(virCommandPtr cmd,
> virBufferPtr buf);
> @@ -134,8 +134,7 @@ void virCommandAddArgFormat(virCommandPtr cmd,
>
> void virCommandAddArgPair(virCommandPtr cmd,
> const char *name,
> - const char *val)
> - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
> + const char *val);
>
> void virCommandAddArgSet(virCommandPtr cmd,
> const char *const*vals) ATTRIBUTE_NONNULL(2);
Regards,
Daniel