[PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If and interface is removed while properties are pending it would cause
process_properties_from_interface to clear data->pending_prop when it
should only clear the iface->pending_prop.
---
 gdbus/object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index afb458764..5c8ad7a55 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -1659,8 +1659,6 @@ static void process_properties_from_interface(struct generic_data *data,
 	DBusMessageIter iter, dict, array;
 	GSList *invalidated;
 
-	data->pending_prop = FALSE;
-
 	if (iface->pending_prop == NULL)
 		return;
 
@@ -1722,6 +1720,8 @@ static void process_property_changes(struct generic_data *data)
 {
 	GSList *l;
 
+	data->pending_prop = FALSE;
+
 	for (l = data->interfaces; l != NULL; l = l->next) {
 		struct interface_data *iface = l->data;
 
-- 
2.13.6

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Bastien Nocera]

> Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

emitting.

On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> If and interface is removed while properties are pending it would cause

"an interface".

> process_properties_from_interface to clear data->pending_prop when it
> should only clear the iface->pending_prop.

I prefer my version, which allows the pending properties to be cleared
out if the pending ones were for that interface. I also prefer the
subject line I used, describing the actual problem it fixes, and the
commit message going more into details.

But up to you.

Cheers

> ---
>  gdbus/object.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbus/object.c b/gdbus/object.c
> index afb458764..5c8ad7a55 100644
> --- a/gdbus/object.c
> +++ b/gdbus/object.c
> @@ -1659,8 +1659,6 @@ static void
> process_properties_from_interface(struct generic_data *data,
>  	DBusMessageIter iter, dict, array;
>  	GSList *invalidated;
>  
> -	data->pending_prop = FALSE;
> -
>  	if (iface->pending_prop == NULL)
>  		return;
>  
> @@ -1722,6 +1720,8 @@ static void process_property_changes(struct
> generic_data *data)
>  {
>  	GSList *l;
>  
> +	data->pending_prop = FALSE;
> +
>  	for (l = data->interfaces; l != NULL; l = l->next) {
>  		struct interface_data *iface = l->data;
>  

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Luiz Augusto von Dentz]

Hi Bastien,

On Wed, Nov 22, 2017 at 12:05 AM, Bastien Nocera <hadess@hadess.net> wrote:
>> Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged
>
> emitting.
>
> On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz wrote:
>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>
>> If and interface is removed while properties are pending it would cause
>
> "an interface".
>
>> process_properties_from_interface to clear data->pending_prop when it
>> should only clear the iface->pending_prop.
>
> I prefer my version, which allows the pending properties to be cleared
> out if the pending ones were for that interface. I also prefer the
> subject line I used, describing the actual problem it fixes, and the
> commit message going more into details.

I guess this one is simpler to maintain, also notice that iterating in
the data->interfaces to check if there are no other pending_prop has
almost the same effect as process_property_changes, what would make a
difference would be to prevent process_changes but that has to run
anyway to propagate the interfaces added/removed. Also notice that
data->pending_prop is clear earlier than the original fix for
duplicated signals, the one that introduced this problem, that way we
don't reintroduce the original problem.

Regarding the commit message, we usually avoid BlueZ specific
description there as gdbus is shared with other projects.

>
> But up to you.
>
> Cheers
>
>> ---
>>  gdbus/object.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbus/object.c b/gdbus/object.c
>> index afb458764..5c8ad7a55 100644
>> --- a/gdbus/object.c
>> +++ b/gdbus/object.c
>> @@ -1659,8 +1659,6 @@ static void
>> process_properties_from_interface(struct generic_data *data,
>>       DBusMessageIter iter, dict, array;
>>       GSList *invalidated;
>>
>> -     data->pending_prop = FALSE;
>> -
>>       if (iface->pending_prop == NULL)
>>               return;
>>
>> @@ -1722,6 +1720,8 @@ static void process_property_changes(struct
>> generic_data *data)
>>  {
>>       GSList *l;
>>
>> +     data->pending_prop = FALSE;
>> +
>>       for (l = data->interfaces; l != NULL; l = l->next) {
>>               struct interface_data *iface = l->data;
>>



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Luiz Augusto von Dentz]

Hi,

On Wed, Nov 22, 2017 at 11:14 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Bastien,
>
> On Wed, Nov 22, 2017 at 12:05 AM, Bastien Nocera <hadess@hadess.net> wrote:
>>> Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged
>>
>> emitting.
>>
>> On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz wrote:
>>> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>>
>>> If and interface is removed while properties are pending it would cause
>>
>> "an interface".
>>
>>> process_properties_from_interface to clear data->pending_prop when it
>>> should only clear the iface->pending_prop.
>>
>> I prefer my version, which allows the pending properties to be cleared
>> out if the pending ones were for that interface. I also prefer the
>> subject line I used, describing the actual problem it fixes, and the
>> commit message going more into details.
>
> I guess this one is simpler to maintain, also notice that iterating in
> the data->interfaces to check if there are no other pending_prop has
> almost the same effect as process_property_changes, what would make a
> difference would be to prevent process_changes but that has to run
> anyway to propagate the interfaces added/removed. Also notice that
> data->pending_prop is clear earlier than the original fix for
> duplicated signals, the one that introduced this problem, that way we
> don't reintroduce the original problem.
>
> Regarding the commit message, we usually avoid BlueZ specific
> description there as gdbus is shared with other projects.
>
>>
>> But up to you.
>>
>> Cheers
>>
>>> ---
>>>  gdbus/object.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gdbus/object.c b/gdbus/object.c
>>> index afb458764..5c8ad7a55 100644
>>> --- a/gdbus/object.c
>>> +++ b/gdbus/object.c
>>> @@ -1659,8 +1659,6 @@ static void
>>> process_properties_from_interface(struct generic_data *data,
>>>       DBusMessageIter iter, dict, array;
>>>       GSList *invalidated;
>>>
>>> -     data->pending_prop = FALSE;
>>> -
>>>       if (iface->pending_prop == NULL)
>>>               return;
>>>
>>> @@ -1722,6 +1720,8 @@ static void process_property_changes(struct
>>> generic_data *data)
>>>  {
>>>       GSList *l;
>>>
>>> +     data->pending_prop = FALSE;
>>> +
>>>       for (l = data->interfaces; l != NULL; l = l->next) {
>>>               struct interface_data *iface = l->data;
>>>

Applied.

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Bastien Nocera]

On Wed, 2017-11-22 at 15:38 +0200, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Wed, Nov 22, 2017 at 11:14 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > Hi Bastien,
> > 
> > On Wed, Nov 22, 2017 at 12:05 AM, Bastien Nocera <hadess@hadess.net
> > > wrote:
> > > > Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged
> > > 
> > > emitting.
> > > 
> > > On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz wrote:
> > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > 
> > > > If and interface is removed while properties are pending it
> > > > would cause
> > > 
> > > "an interface".
> > > 
> > > > process_properties_from_interface to clear data->pending_prop
> > > > when it
> > > > should only clear the iface->pending_prop.
> > > 
> > > I prefer my version, which allows the pending properties to be
> > > cleared
> > > out if the pending ones were for that interface. I also prefer
> > > the
> > > subject line I used, describing the actual problem it fixes, and
> > > the
> > > commit message going more into details.
> > 
> > I guess this one is simpler to maintain, also notice that iterating
> > in
> > the data->interfaces to check if there are no other pending_prop
> > has
> > almost the same effect as process_property_changes, what would make
> > a
> > difference would be to prevent process_changes but that has to run
> > anyway to propagate the interfaces added/removed. Also notice that
> > data->pending_prop is clear earlier than the original fix for
> > duplicated signals, the one that introduced this problem, that way
> > we
> > don't reintroduce the original problem.
> > 
> > Regarding the commit message, we usually avoid BlueZ specific
> > description there as gdbus is shared with other projects.
> > 
> > > 
> > > But up to you.
> > > 
> > > Cheers
> > > 
> > > > ---
> > > >  gdbus/object.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/gdbus/object.c b/gdbus/object.c
> > > > index afb458764..5c8ad7a55 100644
> > > > --- a/gdbus/object.c
> > > > +++ b/gdbus/object.c
> > > > @@ -1659,8 +1659,6 @@ static void
> > > > process_properties_from_interface(struct generic_data *data,
> > > >       DBusMessageIter iter, dict, array;
> > > >       GSList *invalidated;
> > > > 
> > > > -     data->pending_prop = FALSE;
> > > > -
> > > >       if (iface->pending_prop == NULL)
> > > >               return;
> > > > 
> > > > @@ -1722,6 +1720,8 @@ static void
> > > > process_property_changes(struct
> > > > generic_data *data)
> > > >  {
> > > >       GSList *l;
> > > > 
> > > > +     data->pending_prop = FALSE;
> > > > +
> > > >       for (l = data->interfaces; l != NULL; l = l->next) {
> > > >               struct interface_data *iface = l->data;
> > > > 
> 
> Applied.

You could at least have applied fixes to the 2 typos I mentioned :(

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Luiz Augusto von Dentz]

Hi Bastien,

On Wed, Nov 22, 2017 at 3:39 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Wed, 2017-11-22 at 15:38 +0200, Luiz Augusto von Dentz wrote:
>> Hi,
>>
>> On Wed, Nov 22, 2017 at 11:14 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>> > Hi Bastien,
>> >
>> > On Wed, Nov 22, 2017 at 12:05 AM, Bastien Nocera <hadess@hadess.net
>> > > wrote:
>> > > > Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged
>> > >
>> > > emitting.
>> > >
>> > > On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz wrote:
>> > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> > > >
>> > > > If and interface is removed while properties are pending it
>> > > > would cause
>> > >
>> > > "an interface".
>> > >
>> > > > process_properties_from_interface to clear data->pending_prop
>> > > > when it
>> > > > should only clear the iface->pending_prop.
>> > >
>> > > I prefer my version, which allows the pending properties to be
>> > > cleared
>> > > out if the pending ones were for that interface. I also prefer
>> > > the
>> > > subject line I used, describing the actual problem it fixes, and
>> > > the
>> > > commit message going more into details.
>> >
>> > I guess this one is simpler to maintain, also notice that iterating
>> > in
>> > the data->interfaces to check if there are no other pending_prop
>> > has
>> > almost the same effect as process_property_changes, what would make
>> > a
>> > difference would be to prevent process_changes but that has to run
>> > anyway to propagate the interfaces added/removed. Also notice that
>> > data->pending_prop is clear earlier than the original fix for
>> > duplicated signals, the one that introduced this problem, that way
>> > we
>> > don't reintroduce the original problem.
>> >
>> > Regarding the commit message, we usually avoid BlueZ specific
>> > description there as gdbus is shared with other projects.
>> >
>> > >
>> > > But up to you.
>> > >
>> > > Cheers
>> > >
>> > > > ---
>> > > >  gdbus/object.c | 4 ++--
>> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/gdbus/object.c b/gdbus/object.c
>> > > > index afb458764..5c8ad7a55 100644
>> > > > --- a/gdbus/object.c
>> > > > +++ b/gdbus/object.c
>> > > > @@ -1659,8 +1659,6 @@ static void
>> > > > process_properties_from_interface(struct generic_data *data,
>> > > >       DBusMessageIter iter, dict, array;
>> > > >       GSList *invalidated;
>> > > >
>> > > > -     data->pending_prop = FALSE;
>> > > > -
>> > > >       if (iface->pending_prop == NULL)
>> > > >               return;
>> > > >
>> > > > @@ -1722,6 +1720,8 @@ static void
>> > > > process_property_changes(struct
>> > > > generic_data *data)
>> > > >  {
>> > > >       GSList *l;
>> > > >
>> > > > +     data->pending_prop = FALSE;
>> > > > +
>> > > >       for (l = data->interfaces; l != NULL; l = l->next) {
>> > > >               struct interface_data *iface = l->data;
>> > > >
>>
>> Applied.
>
> You could at least have applied fixes to the 2 typos I mentioned :(

You mean the one typo in the description, and instead of an, or did I
miss something in the code?

-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged

[Bastien Nocera]

On Wed, 2017-11-22 at 15:43 +0200, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Wed, Nov 22, 2017 at 3:39 PM, Bastien Nocera <hadess@hadess.net>
> wrote:
> > On Wed, 2017-11-22 at 15:38 +0200, Luiz Augusto von Dentz wrote:
> > > Hi,
> > > 
> > > On Wed, Nov 22, 2017 at 11:14 AM, Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > > Hi Bastien,
> > > > 
> > > > On Wed, Nov 22, 2017 at 12:05 AM, Bastien Nocera <hadess@hadess
> > > > .net
> > > > > wrote:
> > > > > > Re: [PATCH BlueZ] gdbus: Fix not emiting PropertiesChanged
> > > > > 
> > > > > emitting.
> > > > > 
> > > > > On Tue, 2017-11-21 at 22:04 +0200, Luiz Augusto von Dentz
> > > > > wrote:
> > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > > 
> > > > > > If and interface is removed while properties are pending it
> > > > > > would cause
> > > > > 
> > > > > "an interface".
> > > > > 
> > > > > > process_properties_from_interface to clear data-
> > > > > > >pending_prop
> > > > > > when it
> > > > > > should only clear the iface->pending_prop.
> > > > > 
> > > > > I prefer my version, which allows the pending properties to
> > > > > be
> > > > > cleared
> > > > > out if the pending ones were for that interface. I also
> > > > > prefer
> > > > > the
> > > > > subject line I used, describing the actual problem it fixes,
> > > > > and
> > > > > the
> > > > > commit message going more into details.
> > > > 
> > > > I guess this one is simpler to maintain, also notice that
> > > > iterating
> > > > in
> > > > the data->interfaces to check if there are no other
> > > > pending_prop
> > > > has
> > > > almost the same effect as process_property_changes, what would
> > > > make
> > > > a
> > > > difference would be to prevent process_changes but that has to
> > > > run
> > > > anyway to propagate the interfaces added/removed. Also notice
> > > > that
> > > > data->pending_prop is clear earlier than the original fix for
> > > > duplicated signals, the one that introduced this problem, that
> > > > way
> > > > we
> > > > don't reintroduce the original problem.
> > > > 
> > > > Regarding the commit message, we usually avoid BlueZ specific
> > > > description there as gdbus is shared with other projects.
> > > > 
> > > > > 
> > > > > But up to you.
> > > > > 
> > > > > Cheers
> > > > > 
> > > > > > ---
> > > > > >  gdbus/object.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/gdbus/object.c b/gdbus/object.c
> > > > > > index afb458764..5c8ad7a55 100644
> > > > > > --- a/gdbus/object.c
> > > > > > +++ b/gdbus/object.c
> > > > > > @@ -1659,8 +1659,6 @@ static void
> > > > > > process_properties_from_interface(struct generic_data
> > > > > > *data,
> > > > > >       DBusMessageIter iter, dict, array;
> > > > > >       GSList *invalidated;
> > > > > > 
> > > > > > -     data->pending_prop = FALSE;
> > > > > > -
> > > > > >       if (iface->pending_prop == NULL)
> > > > > >               return;
> > > > > > 
> > > > > > @@ -1722,6 +1720,8 @@ static void
> > > > > > process_property_changes(struct
> > > > > > generic_data *data)
> > > > > >  {
> > > > > >       GSList *l;
> > > > > > 
> > > > > > +     data->pending_prop = FALSE;
> > > > > > +
> > > > > >       for (l = data->interfaces; l != NULL; l = l->next) {
> > > > > >               struct interface_data *iface = l->data;
> > > > > > 
> > > 
> > > Applied.
> > 
> > You could at least have applied fixes to the 2 typos I mentioned :(
> 
> You mean the one typo in the description, and instead of an, or did I
> miss something in the code?

There's a typo in the commit subject and another in the commit body. I
mentioned both of those in my initial review.