Re: [PATCH 2/5] dbus: Coalesce PropertiesChanged signals

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

Hi Andrew,

On 02/29/2016 03:21 AM, Andrzej Zaborowski wrote:
> Hi Denis,
>
> On 26 February 2016 at 21:30, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 02/17/2016 09:20 PM, Andrew Zaborowski wrote:
>>>          if (!property)
>>>                  return false;
>>>
>>> -       signature = property->metainfo + strlen(property->metainfo) + 1;
>>> -
>>> -       if (instance->interface->handle_old_style_properties) {
>>> -               signal = l_dbus_message_new_signal(dbus, path,
>>> -                                                       interface_name,
>>> -
>>> "PropertyChanged");
>>> +       rec = l_queue_find(tree->property_changes,
>>> +                               match_property_changes_instance,
>>> instance);
>>>

The l_queue_find call only takes the instance into consideration, not 
the object pointer.

>>> -               builder = l_dbus_message_builder_new(signal);
>>> -
>>> -               l_dbus_message_builder_append_basic(builder, 's',
>>> -                                                       property_name);
>>> -               l_dbus_message_builder_enter_variant(builder, signature);
>>
>>
>> This looks a bit suspicious.  Should we be also comparing the object
>> pointer, not just the instance pointer here?
>
> Assuming that only one object can have the instance it shouldn't
> matter.  When the object or the instance are removed, the property
> change record is also removed so it won't accidentally point at a
> different instance.
>
> We also rely on that for the interface added notifications.
>

What about multiple objects with the same interface?

E.g. org.foo.Foobar interface on /bar1 and /bar2.  If Foobar.Baz 
property changes on /bar1 and /bar2.

Alternatively, if Foobar.Baz1 changes on /bar1, and Foobar.Baz2 changes 
on /bar2, you can also get into a weird situation.

Or am I missing something?

Regards,
-Denis