hostdev: Fix build with GCC's static analysis in mdev

[libvirt] [PATCH] hostdev: Fix build with GCC's static analysis in mdev

Posted by Martin Kletzander 7 years ago

Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
lv_cv_static_analysis=yes.

Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
---
 src/util/virhostdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 2c557f5bbc6b..998df5871539 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -2060,8 +2060,8 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr,
             continue;

         virMediatedDeviceGetUsedBy(tmp, &used_by_drvname, &used_by_domname);
-        if (STREQ_NULLABLE(drv_name, used_by_drvname) &&
-            STREQ_NULLABLE(dom_name, used_by_domname)) {
+        if (used_by_drvname && STREQ(drv_name, used_by_drvname) &&
+            used_by_domname && STREQ(dom_name, used_by_domname)) {
             VIR_DEBUG("Removing %s dom=%s from activeMediatedHostdevs",
                       mdevsrc->uuidstr, dom_name);
             virMediatedDeviceListDel(mgr->activeMediatedHostdevs, tmp);
-- 
2.12.1

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

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

Posted by John Ferlan 7 years ago


On 03/27/2017 11:30 AM, Martin Kletzander wrote:
> Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
> lv_cv_static_analysis=yes.
> 
> Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
> ---
>  src/util/virhostdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Sure that's another way to fix it... Could have also gone with the
removal of NONNULL in the prototype like I did.  IDC whichever way is
deemed "more appropriate"...

Seeing as drv_name is only ever passed as QEMU_DRIVER_NAME, but dom_name
is passed from 'name' which doesn't have the NONNULL on it, that's why I
chose removing NONNULL from the prototype.

John

> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 2c557f5bbc6b..998df5871539 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -2060,8 +2060,8 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr,
>              continue;
> 
>          virMediatedDeviceGetUsedBy(tmp, &used_by_drvname, &used_by_domname);
> -        if (STREQ_NULLABLE(drv_name, used_by_drvname) &&
> -            STREQ_NULLABLE(dom_name, used_by_domname)) {
> +        if (used_by_drvname && STREQ(drv_name, used_by_drvname) &&
> +            used_by_domname && STREQ(dom_name, used_by_domname)) {
>              VIR_DEBUG("Removing %s dom=%s from activeMediatedHostdevs",
>                        mdevsrc->uuidstr, dom_name);
>              virMediatedDeviceListDel(mgr->activeMediatedHostdevs, tmp);
> 

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

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

Posted by Laine Stump 7 years ago

On 03/27/2017 11:40 AM, John Ferlan wrote:
> 
> 
> On 03/27/2017 11:30 AM, Martin Kletzander wrote:
>> Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
>> lv_cv_static_analysis=yes.
>>
>> Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
>> ---
>>  src/util/virhostdev.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
> 
> Sure that's another way to fix it... Could have also gone with the
> removal of NONNULL in the prototype like I did.  IDC whichever way is
> deemed "more appropriate"...
> 
> Seeing as drv_name is only ever passed as QEMU_DRIVER_NAME, but dom_name
> is passed from 'name' which doesn't have the NONNULL on it, that's why I
> chose removing NONNULL from the prototype.

I think I prefer John's way too. (At least partly because I dislike
ATTRIBUTE_NONNULL() and would like to see as many of them as possible go
away).

> 
> John
> 
>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>> index 2c557f5bbc6b..998df5871539 100644
>> --- a/src/util/virhostdev.c
>> +++ b/src/util/virhostdev.c
>> @@ -2060,8 +2060,8 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr,
>>              continue;
>>
>>          virMediatedDeviceGetUsedBy(tmp, &used_by_drvname, &used_by_domname);
>> -        if (STREQ_NULLABLE(drv_name, used_by_drvname) &&
>> -            STREQ_NULLABLE(dom_name, used_by_domname)) {
>> +        if (used_by_drvname && STREQ(drv_name, used_by_drvname) &&
>> +            used_by_domname && STREQ(dom_name, used_by_domname)) {
>>              VIR_DEBUG("Removing %s dom=%s from activeMediatedHostdevs",
>>                        mdevsrc->uuidstr, dom_name);
>>              virMediatedDeviceListDel(mgr->activeMediatedHostdevs, tmp);
>>
> 
> --
> 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] hostdev: Fix build with GCC's static analysis in mdev

Posted by Martin Kletzander 7 years ago

On Mon, Mar 27, 2017 at 12:30:23PM -0400, Laine Stump wrote:
>On 03/27/2017 11:40 AM, John Ferlan wrote:
>>
>>
>> On 03/27/2017 11:30 AM, Martin Kletzander wrote:
>>> Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
>>> lv_cv_static_analysis=yes.
>>>
>>> Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
>>> ---
>>>  src/util/virhostdev.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Sure that's another way to fix it... Could have also gone with the
>> removal of NONNULL in the prototype like I did.  IDC whichever way is
>> deemed "more appropriate"...
>>
>> Seeing as drv_name is only ever passed as QEMU_DRIVER_NAME, but dom_name
>> is passed from 'name' which doesn't have the NONNULL on it, that's why I
>> chose removing NONNULL from the prototype.
>
>I think I prefer John's way too. (At least partly because I dislike
>ATTRIBUTE_NONNULL() and would like to see as many of them as possible go
>away).
>

Actually, I do too.  I would love to remove all of them.  But for that
we'd have to reach a decision.  However there are not only cons, some
pros can be that the compiler is able to make better guesses or not have
to be guessing at all.  As a counter-argument, there are way more
attribute we could use and we don't, plus libvirt is not the CPU hogger
where we would have to go optimize so low.  So while the answer is not
clear, I wouldn't, as mentioned before, be against removing all such
occurrences, maybe except public APIs, if someone happens to have a good
reason for that.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Martin Kletzander 7 years ago

On Mon, Mar 27, 2017 at 08:24:58PM +0200, Martin Kletzander wrote:
>On Mon, Mar 27, 2017 at 12:30:23PM -0400, Laine Stump wrote:
>>On 03/27/2017 11:40 AM, John Ferlan wrote:
>>>
>>>
>>> On 03/27/2017 11:30 AM, Martin Kletzander wrote:
>>>> Similarly to eec3b255d26e7b38bdb0830990569fd91aee661f, fix build with
>>>> lv_cv_static_analysis=yes.
>>>>
>>>> Caused by a4a39d90ab4930750bcbcfccffdf6bb6d310b5d5
>>>> ---
>>>>  src/util/virhostdev.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Sure that's another way to fix it... Could have also gone with the
>>> removal of NONNULL in the prototype like I did.  IDC whichever way is
>>> deemed "more appropriate"...
>>>
>>> Seeing as drv_name is only ever passed as QEMU_DRIVER_NAME, but dom_name
>>> is passed from 'name' which doesn't have the NONNULL on it, that's why I
>>> chose removing NONNULL from the prototype.
>>
>>I think I prefer John's way too. (At least partly because I dislike
>>ATTRIBUTE_NONNULL() and would like to see as many of them as possible go
>>away).
>>
>
>Actually, I do too.  I would love to remove all of them.  But for that
>we'd have to reach a decision.  However there are not only cons, some
>pros can be that the compiler is able to make better guesses or not have
>to be guessing at all.  As a counter-argument, there are way more
>attribute we could use and we don't, plus libvirt is not the CPU hogger
>where we would have to go optimize so low.  So while the answer is not
>clear, I wouldn't, as mentioned before, be against removing all such
>occurrences, maybe except public APIs, if someone happens to have a good
>reason for that.

I see your commit from 2012 now.  And I think we might consider removing
attribute_nonnull altogether.  Let me propose a patch...

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