Fix build with GCC's static analysis

[libvirt] [PATCH] Fix build with GCC's static analysis

Posted by Martin Kletzander 7 years ago

STREQ_NULLABLE returns true if both parameters are NULL.  And that's
not what we want here.  We just want to skop comparing source nodes
that don't have that info set.  The function wouldn't make much sense
with nodeName == NULL, so we don't need to check that.  Moreover, the
function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not
only means that function expects the parameter not to be NULL, but
actually tells the compiler that it can optimize out the NULL checks.
That way it could end up calling strcmp on NULL (either nodeformat or
nodebacking).  GCC figures this out if libvirt is compiled with
lv_cv_static_analysis=yes, unfortunately not everyone uses that.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virstoragefile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 3bcb69bf6206..0ac707962102 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -3809,8 +3809,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top,
         *index = 0;

     for (tmp = top; tmp; tmp = tmp->backingStore) {
-        if (STREQ_NULLABLE(tmp->nodeformat, nodeName) ||
-            STREQ_NULLABLE(tmp->nodebacking, nodeName))
+        if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
+            (tmp->nodebacking && STREQ(tmp->nodebacking, nodeName)))
             return tmp;

         if (index)
-- 
2.12.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix build with GCC's static analysis

Posted by John Ferlan 7 years ago


On 03/27/2017 06:46 AM, Martin Kletzander wrote:
> STREQ_NULLABLE returns true if both parameters are NULL.  And that's
> not what we want here.  We just want to skop comparing source nodes

stop

> that don't have that info set.  The function wouldn't make much sense
> with nodeName == NULL, so we don't need to check that.  Moreover, the
> function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not

ATTRIBUTE

> only means that function expects the parameter not to be NULL, but
> actually tells the compiler that it can optimize out the NULL checks.
> That way it could end up calling strcmp on NULL (either nodeformat or
> nodebacking).  GCC figures this out if libvirt is compiled with
> lv_cv_static_analysis=yes, unfortunately not everyone uses that.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/util/virstoragefile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

ACK

John

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 3bcb69bf6206..0ac707962102 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -3809,8 +3809,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top,
>          *index = 0;
> 
>      for (tmp = top; tmp; tmp = tmp->backingStore) {
> -        if (STREQ_NULLABLE(tmp->nodeformat, nodeName) ||
> -            STREQ_NULLABLE(tmp->nodebacking, nodeName))
> +        if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
> +            (tmp->nodebacking && STREQ(tmp->nodebacking, nodeName)))
>              return tmp;
> 
>          if (index)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix build with GCC's static analysis

Posted by Martin Kletzander 7 years ago

On Mon, Mar 27, 2017 at 06:57:10AM -0400, John Ferlan wrote:
>
>
>On 03/27/2017 06:46 AM, Martin Kletzander wrote:
>> STREQ_NULLABLE returns true if both parameters are NULL.  And that's
>> not what we want here.  We just want to skop comparing source nodes
>
>stop
>

oh

>> that don't have that info set.  The function wouldn't make much sense
>> with nodeName == NULL, so we don't need to check that.  Moreover, the
>> function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not
>
>ATTRIBUTE
>

OH

How do I words again?

>> only means that function expects the parameter not to be NULL, but
>> actually tells the compiler that it can optimize out the NULL checks.
>> That way it could end up calling strcmp on NULL (either nodeformat or
>> nodebacking).  GCC figures this out if libvirt is compiled with
>> lv_cv_static_analysis=yes, unfortunately not everyone uses that.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/util/virstoragefile.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>
>ACK
>

Thanks, will push in a while.  I also added the commit ID for back-reference.

>John
>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 3bcb69bf6206..0ac707962102 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -3809,8 +3809,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top,
>>          *index = 0;
>>
>>      for (tmp = top; tmp; tmp = tmp->backingStore) {
>> -        if (STREQ_NULLABLE(tmp->nodeformat, nodeName) ||
>> -            STREQ_NULLABLE(tmp->nodebacking, nodeName))
>> +        if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
>> +            (tmp->nodebacking && STREQ(tmp->nodebacking, nodeName)))
>>              return tmp;
>>
>>          if (index)
>>
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix build with GCC's static analysis

Posted by Peter Krempa 7 years ago

On Mon, Mar 27, 2017 at 12:46:40 +0200, Martin Kletzander wrote:
> STREQ_NULLABLE returns true if both parameters are NULL.  And that's
> not what we want here.  We just want to skop comparing source nodes

s/skop/skip/

> that don't have that info set.  The function wouldn't make much sense
> with nodeName == NULL, so we don't need to check that.  Moreover, the
> function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not

s/ATTRIBUDE_NONNULL/ATTRIBUTE_NONNULL/

> only means that function expects the parameter not to be NULL, but
> actually tells the compiler that it can optimize out the NULL checks.
> That way it could end up calling strcmp on NULL (either nodeformat or
> nodebacking).  GCC figures this out if libvirt is compiled with
> lv_cv_static_analysis=yes, unfortunately not everyone uses that.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/util/virstoragefile.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 3bcb69bf6206..0ac707962102 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -3809,8 +3809,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top,
>          *index = 0;
> 
>      for (tmp = top; tmp; tmp = tmp->backingStore) {
> -        if (STREQ_NULLABLE(tmp->nodeformat, nodeName) ||
> -            STREQ_NULLABLE(tmp->nodebacking, nodeName))
> +        if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
> +            (tmp->nodebacking && STREQ(tmp->nodebacking, nodeName)))
>              return tmp;
> 

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Fix build with GCC's static analysis

Posted by Martin Kletzander 7 years ago

On Mon, Mar 27, 2017 at 01:21:07PM +0200, Peter Krempa wrote:
>On Mon, Mar 27, 2017 at 12:46:40 +0200, Martin Kletzander wrote:
>> STREQ_NULLABLE returns true if both parameters are NULL.  And that's
>> not what we want here.  We just want to skop comparing source nodes
>
>s/skop/skip/
>
>> that don't have that info set.  The function wouldn't make much sense
>> with nodeName == NULL, so we don't need to check that.  Moreover, the
>> function's declaration uses ATTRIBUDE_NONNULL for nodeName, which not
>
>s/ATTRIBUDE_NONNULL/ATTRIBUTE_NONNULL/
>

Yes, John pointed that out... And I forgot to fix it before pushing...
Where are the ears?

>> only means that function expects the parameter not to be NULL, but
>> actually tells the compiler that it can optimize out the NULL checks.
>> That way it could end up calling strcmp on NULL (either nodeformat or
>> nodebacking).  GCC figures this out if libvirt is compiled with
>> lv_cv_static_analysis=yes, unfortunately not everyone uses that.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>  src/util/virstoragefile.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 3bcb69bf6206..0ac707962102 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -3809,8 +3809,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top,
>>          *index = 0;
>>
>>      for (tmp = top; tmp; tmp = tmp->backingStore) {
>> -        if (STREQ_NULLABLE(tmp->nodeformat, nodeName) ||
>> -            STREQ_NULLABLE(tmp->nodebacking, nodeName))
>> +        if ((tmp->nodeformat && STREQ(tmp->nodeformat, nodeName)) ||
>> +            (tmp->nodebacking && STREQ(tmp->nodebacking, nodeName)))
>>              return tmp;
>>
>
>ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list