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@xxxxxxxxxx>
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > > >
> > > > > > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
