[v2,2/8] conf: add node_device_event handling

Message ID 66ae190f-827e-0f5d-b4d9-1692dea5507e@redhat.com
State New
Headers show

Commit Message

Cole Robinson Aug. 3, 2016, 9:37 p.m.
On 08/03/2016 09:40 AM, John Ferlan wrote:
> 

> 

> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:

>> Add node device event handling infrastructure to node_device_event.[ch]

>> ---

>>  src/Makefile.am              |   5 +

>>  src/conf/node_device_event.c | 234 +++++++++++++++++++++++++++++++++++++++++++

>>  src/conf/node_device_event.h |  59 +++++++++++

>>  src/libvirt_private.syms     |   5 +

>>  4 files changed, 303 insertions(+)

>>  create mode 100644 src/conf/node_device_event.c

>>  create mode 100644 src/conf/node_device_event.h

>>

> 

> [...]

> 

>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c

>> new file mode 100644

>> index 0000000..61bc912

> 

> [...]

> 

>> +

>> +/**

>> + * virNodeDeviceEventLifecycleNew:

>> + * @name: name of the node device object the event describes

>> + * @type: type of lifecycle event

>> + * @detail: more details about @type

>> + *

>> + * Create a new node device lifecycle event.

>> + */

>> +virObjectEventPtr

>> +virNodeDeviceEventLifecycleNew(const char *name,

>> +                               int type,

>> +                               int detail)

>> +{

>> +    virNodeDeviceEventLifecyclePtr event;

>> +

>> +    if (virNodeDeviceEventsInitialize() < 0)

>> +        return NULL;

>> +

>> +    if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,

>> +                                    virNodeDeviceEventDispatchDefaultFunc,

>> +                                    VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,

>> +                                    0, name, NULL, name)))

>                                                 ^^^^

> 

> This has caused a Coverity build failure since the prototype has:

> 

> ATTRIBUTE_NONNULL(6)

> 


I think just dropping it is fine? The code was updated to handle uuid=NULL


> It gets even worse in the function and needs to be resolved before the

> "next" release.

> 


I can't parse this sentence... are there additional issues?

Thanks,
Cole

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

Comments

Cole Robinson Aug. 3, 2016, 10:30 p.m. | #1
On 08/03/2016 05:57 PM, John Ferlan wrote:
> 

> 

> On 08/03/2016 05:37 PM, Cole Robinson wrote:

>> On 08/03/2016 09:40 AM, John Ferlan wrote:

>>>

>>>

>>> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:

>>>> Add node device event handling infrastructure to node_device_event.[ch]

>>>> ---

>>>>  src/Makefile.am              |   5 +

>>>>  src/conf/node_device_event.c | 234 +++++++++++++++++++++++++++++++++++++++++++

>>>>  src/conf/node_device_event.h |  59 +++++++++++

>>>>  src/libvirt_private.syms     |   5 +

>>>>  4 files changed, 303 insertions(+)

>>>>  create mode 100644 src/conf/node_device_event.c

>>>>  create mode 100644 src/conf/node_device_event.h

>>>>

>>>

>>> [...]

>>>

>>>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c

>>>> new file mode 100644

>>>> index 0000000..61bc912

>>>

>>> [...]

>>>

>>>> +

>>>> +/**

>>>> + * virNodeDeviceEventLifecycleNew:

>>>> + * @name: name of the node device object the event describes

>>>> + * @type: type of lifecycle event

>>>> + * @detail: more details about @type

>>>> + *

>>>> + * Create a new node device lifecycle event.

>>>> + */

>>>> +virObjectEventPtr

>>>> +virNodeDeviceEventLifecycleNew(const char *name,

>>>> +                               int type,

>>>> +                               int detail)

>>>> +{

>>>> +    virNodeDeviceEventLifecyclePtr event;

>>>> +

>>>> +    if (virNodeDeviceEventsInitialize() < 0)

>>>> +        return NULL;

>>>> +

>>>> +    if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,

>>>> +                                    virNodeDeviceEventDispatchDefaultFunc,

>>>> +                                    VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,

>>>> +                                    0, name, NULL, name)))

>>>                                                 ^^^^

>>>

>>> This has caused a Coverity build failure since the prototype has:

>>>

>>> ATTRIBUTE_NONNULL(6)

>>>

>>

>> I think just dropping it is fine? The code was updated to handle uuid=NULL

>>

>> diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h

>> index 92c25d4..27b461f 100644

>> --- a/src/conf/object_event_private.h

>> +++ b/src/conf/object_event_private.h

>> @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,

>>                    const unsigned char *uuid,

>>                    const char *key)

>>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)

>> -    ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);

>> +    ATTRIBUTE_NONNULL(7);

>>

>>  #endif

>>

>>> It gets even worse in the function and needs to be resolved before the

>>> "next" release.

>>>

>>

>> I can't parse this sentence... are there additional issues?

>>

> virObjectEventNew(...,const unsigned char *uuid,...)

> ...

>     memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);

> ...

> 

> e.g.

>     memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);

> 


But the full code from virObjectEventNew handles that case by checking for
non-NULL uuid first:

    if (uuid)
        memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);

- Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 3, 2016, 10:53 p.m. | #2
On 08/03/2016 06:39 PM, John Ferlan wrote:
> 

> 

> On 08/03/2016 06:30 PM, Cole Robinson wrote:

>> On 08/03/2016 05:57 PM, John Ferlan wrote:

>>>

>>>

>>> On 08/03/2016 05:37 PM, Cole Robinson wrote:

>>>> On 08/03/2016 09:40 AM, John Ferlan wrote:

>>>>>

>>>>>

>>>>> On 07/28/2016 08:02 AM, Jovanka Gulicoska wrote:

>>>>>> Add node device event handling infrastructure to node_device_event.[ch]

>>>>>> ---

>>>>>>  src/Makefile.am              |   5 +

>>>>>>  src/conf/node_device_event.c | 234 +++++++++++++++++++++++++++++++++++++++++++

>>>>>>  src/conf/node_device_event.h |  59 +++++++++++

>>>>>>  src/libvirt_private.syms     |   5 +

>>>>>>  4 files changed, 303 insertions(+)

>>>>>>  create mode 100644 src/conf/node_device_event.c

>>>>>>  create mode 100644 src/conf/node_device_event.h

>>>>>>

>>>>>

>>>>> [...]

>>>>>

>>>>>> diff --git a/src/conf/node_device_event.c b/src/conf/node_device_event.c

>>>>>> new file mode 100644

>>>>>> index 0000000..61bc912

>>>>>

>>>>> [...]

>>>>>

>>>>>> +

>>>>>> +/**

>>>>>> + * virNodeDeviceEventLifecycleNew:

>>>>>> + * @name: name of the node device object the event describes

>>>>>> + * @type: type of lifecycle event

>>>>>> + * @detail: more details about @type

>>>>>> + *

>>>>>> + * Create a new node device lifecycle event.

>>>>>> + */

>>>>>> +virObjectEventPtr

>>>>>> +virNodeDeviceEventLifecycleNew(const char *name,

>>>>>> +                               int type,

>>>>>> +                               int detail)

>>>>>> +{

>>>>>> +    virNodeDeviceEventLifecyclePtr event;

>>>>>> +

>>>>>> +    if (virNodeDeviceEventsInitialize() < 0)

>>>>>> +        return NULL;

>>>>>> +

>>>>>> +    if (!(event = virObjectEventNew(virNodeDeviceEventLifecycleClass,

>>>>>> +                                    virNodeDeviceEventDispatchDefaultFunc,

>>>>>> +                                    VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE,

>>>>>> +                                    0, name, NULL, name)))

>>>>>                                                 ^^^^

>>>>>

>>>>> This has caused a Coverity build failure since the prototype has:

>>>>>

>>>>> ATTRIBUTE_NONNULL(6)

>>>>>

>>>>

>>>> I think just dropping it is fine? The code was updated to handle uuid=NULL

>>>>

>>>> diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h

>>>> index 92c25d4..27b461f 100644

>>>> --- a/src/conf/object_event_private.h

>>>> +++ b/src/conf/object_event_private.h

>>>> @@ -106,6 +106,6 @@ virObjectEventNew(virClassPtr klass,

>>>>                    const unsigned char *uuid,

>>>>                    const char *key)

>>>>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)

>>>> -    ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);

>>>> +    ATTRIBUTE_NONNULL(7);

>>>>

>>>>  #endif

>>>>

>>>>> It gets even worse in the function and needs to be resolved before the

>>>>> "next" release.

>>>>>

>>>>

>>>> I can't parse this sentence... are there additional issues?

>>>>

>>> virObjectEventNew(...,const unsigned char *uuid,...)

>>> ...

>>>     memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);

>>> ...

>>>

>>> e.g.

>>>     memcpy(event->meta.uuid, NULL, VIR_UUID_BUFLEN);

>>>

>>

>> But the full code from virObjectEventNew handles that case by checking for

>> non-NULL uuid first:

>>

>>     if (uuid)

>>         memcpy(event->meta.uuid, uuid, VIR_UUID_BUFLEN);

>>

> 

> Hmm.... strange, my coverity environment AFU'd....  Ahhh I see why.

> 

> OK, removing the ATTRIBUTE_NONNULL(6) works just fine

> 

> ACK for that

> 

> John

> 

> Long story short - recent update to f24 has for some reason caused

> compilation failures for "-Wnonnull-compare"'s within the coverity build

> environment, so I made some local changes to get past it while I figured

> out what the issue was. That was one of the many changes.

> 


Got it. I pushed that change:

commit e584615b81b5cabb252b1866171bde25b4f06d05
Author: Cole Robinson <crobinso@redhat.com>
Date:   Wed Aug 3 18:45:50 2016 -0400

    conf: events: Fix coverity warning

    Since 2bfa75134 virObjectEventNew can be passed a NULL 'uuid' value,
    so drop the ATTRIBUTE_NONNULL annotation

Thanks,
Cole

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

Patch

diff --git a/src/conf/object_event_private.h b/src/conf/object_event_private.h
index 92c25d4..27b461f 100644
--- a/src/conf/object_event_private.h
+++ b/src/conf/object_event_private.h
@@ -106,6 +106,6 @@  virObjectEventNew(virClassPtr klass,
                   const unsigned char *uuid,
                   const char *key)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
-    ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7);
+    ATTRIBUTE_NONNULL(7);

 #endif