[RFC 1/2] gdbus: Add g_dbus_flush_properties()

[RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Vinicius Costa Gomes]

In some situations, it may be useful to be sure that all the
properties updates for a object are sent.
---
 gdbus/gdbus.h  |  2 ++
 gdbus/object.c | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 8b13393..ed280e4 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -299,6 +299,8 @@ void g_dbus_emit_property_changed(DBusConnection *connection,
 gboolean g_dbus_get_properties(DBusConnection *connection, const char *path,
 				const char *interface, DBusMessageIter *iter);
 
+gboolean g_dbus_flush_properties(DBusConnection *connection, const char *path);
+
 gboolean g_dbus_attach_object_manager(DBusConnection *connection);
 gboolean g_dbus_detach_object_manager(DBusConnection *connection);
 
diff --git a/gdbus/object.c b/gdbus/object.c
index 2f8ef45..14b6803 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -1751,6 +1751,25 @@ gboolean g_dbus_get_properties(DBusConnection *connection, const char *path,
 	return TRUE;
 }
 
+gboolean g_dbus_flush_properties(DBusConnection *connection, const char *path)
+{
+	struct generic_data *data;
+
+	if (path == NULL)
+		return FALSE;
+
+	if (!dbus_connection_get_object_path_data(connection, path,
+					(void **) &data) || data == NULL)
+		return FALSE;
+
+	if (data->process_id)
+		g_source_remove(data->process_id);
+
+	process_changes(data);
+
+	return TRUE;
+}
+
 gboolean g_dbus_attach_object_manager(DBusConnection *connection)
 {
 	struct generic_data *data;
-- 
1.8.2.1

[RFC 2/2] device: Fix the wrong order of UUIDs PropertyChanged

[Vinicius Costa Gomes]

The code goes to great length to make sure that the Service Discovery
is finished before sending the NewConnection() message to external
profiles.  In some situations this effort is wasted because the UUIDs
signal is emitted after NewConnection() is sent.

This patch fixes it by making sure that all the properties for that
object are updated before sending the NewConnection() call.
---
 src/device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/device.c b/src/device.c
index ee17514..9f9dba3 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2806,6 +2806,8 @@ static void search_cb(sdp_list_t *recs, int err, gpointer user_data)
 send_reply:
 	device_svc_resolved(device, err);
 
+	g_dbus_flush_properties(dbus_conn, req->device->path);
+
 	if (!device->temporary)
 		store_device_info(device);
 
-- 
1.8.2.1

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Luiz Augusto von Dentz]

Hi Vinicius,

On Thu, Apr 18, 2013 at 8:50 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> In some situations, it may be useful to be sure that all the
> properties updates for a object are sent.
> ---
>  gdbus/gdbus.h  |  2 ++
>  gdbus/object.c | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
> index 8b13393..ed280e4 100644
> --- a/gdbus/gdbus.h
> +++ b/gdbus/gdbus.h
> @@ -299,6 +299,8 @@ void g_dbus_emit_property_changed(DBusConnection *connection,
>  gboolean g_dbus_get_properties(DBusConnection *connection, const char *path,
>                                 const char *interface, DBusMessageIter *iter);
>
> +gboolean g_dbus_flush_properties(DBusConnection *connection, const char *path);
> +
>  gboolean g_dbus_attach_object_manager(DBusConnection *connection);
>  gboolean g_dbus_detach_object_manager(DBusConnection *connection);
>
> diff --git a/gdbus/object.c b/gdbus/object.c
> index 2f8ef45..14b6803 100644
> --- a/gdbus/object.c
> +++ b/gdbus/object.c
> @@ -1751,6 +1751,25 @@ gboolean g_dbus_get_properties(DBusConnection *connection, const char *path,
>         return TRUE;
>  }
>
> +gboolean g_dbus_flush_properties(DBusConnection *connection, const char *path)
> +{
> +       struct generic_data *data;
> +
> +       if (path == NULL)
> +               return FALSE;
> +
> +       if (!dbus_connection_get_object_path_data(connection, path,
> +                                       (void **) &data) || data == NULL)
> +               return FALSE;
> +
> +       if (data->process_id)
> +               g_source_remove(data->process_id);
> +
> +       process_changes(data);
> +
> +       return TRUE;
> +}
> +
>  gboolean g_dbus_attach_object_manager(DBusConnection *connection)
>  {
>         struct generic_data *data;
> --
> 1.8.2.1

What about creating a new flag for properties that need to emit a
signal immediately when they change?


--
Luiz Augusto von Dentz

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Vinicius Costa Gomes]

Hi Luiz,

On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
> Hi Vinicius,
> 

[snip]

> 
> What about creating a new flag for properties that need to emit a
> signal immediately when they change?

That would also solve the problem.

The way I see it, we should still not make any guarantees about when
a propery changed signal will be emitted, only when the API behaviour
requires, we use some kind of barrier to keep the ordering in check.

And having a function to call we make it easier to detect when this
is abused, i.e. we know there's something very wrong if there's a flush
after each emit.

> 
> 
> --
> Luiz Augusto von Dentz


Cheers,
-- 
Vinicius

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Johan Hedberg]

Hi,

On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
> > What about creating a new flag for properties that need to emit a
> > signal immediately when they change?
> 
> That would also solve the problem.
> 
> The way I see it, we should still not make any guarantees about when
> a propery changed signal will be emitted, only when the API behaviour
> requires, we use some kind of barrier to keep the ordering in check.
> 
> And having a function to call we make it easier to detect when this
> is abused, i.e. we know there's something very wrong if there's a flush
> after each emit.

Is this where the discussion got stuck? Since this feature is really
only needed for quite exceptional situations if an API change needs to
be done I'd be in favor of a separate flush function.

Another option is to make *all* D-Bus messages that gdbus sends to use
an idle callback, including method calls and returns. In the idle
callback the messages would be sent in the same order as they were added
to the send queue. This would not require any changes to the gdbus API.

Johan

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Vinicius Costa Gomes]

Hi Johan,

On 16:58 Wed 24 Apr, Johan Hedberg wrote:
> Hi,
> 
> On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
> > On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
> > > What about creating a new flag for properties that need to emit a
> > > signal immediately when they change?
> > 
> > That would also solve the problem.
> > 
> > The way I see it, we should still not make any guarantees about when
> > a propery changed signal will be emitted, only when the API behaviour
> > requires, we use some kind of barrier to keep the ordering in check.
> > 
> > And having a function to call we make it easier to detect when this
> > is abused, i.e. we know there's something very wrong if there's a flush
> > after each emit.
> 
> Is this where the discussion got stuck? Since this feature is really
> only needed for quite exceptional situations if an API change needs to
> be done I'd be in favor of a separate flush function.
> 
> Another option is to make *all* D-Bus messages that gdbus sends to use
> an idle callback, including method calls and returns. In the idle
> callback the messages would be sent in the same order as they were added
> to the send queue. This would not require any changes to the gdbus API.

In this situation we are not using g_dbus_send_message(), we are using
dbus_connection_send_message_with_reply().

> 
> Johan


Cheers,
-- 
Vinicius

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Lucas De Marchi]

On Wed, Apr 24, 2013 at 10:58 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
>> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
>> > What about creating a new flag for properties that need to emit a
>> > signal immediately when they change?
>>
>> That would also solve the problem.
>>
>> The way I see it, we should still not make any guarantees about when
>> a propery changed signal will be emitted, only when the API behaviour
>> requires, we use some kind of barrier to keep the ordering in check.
>>
>> And having a function to call we make it easier to detect when this
>> is abused, i.e. we know there's something very wrong if there's a flush
>> after each emit.
>
> Is this where the discussion got stuck? Since this feature is really
> only needed for quite exceptional situations if an API change needs to
> be done I'd be in favor of a separate flush function.
>
> Another option is to make *all* D-Bus messages that gdbus sends to use
> an idle callback, including method calls and returns. In the idle
> callback the messages would be sent in the same order as they were added
> to the send queue. This would not require any changes to the gdbus API.


ugh.. no. This doesn't play well with sending the message directly
through libdbus.

When we implemented the properties interface, the manner we talked
about doing this (if it was indeed needed) was mostly what Luiz is
saying now. I think I even sent a patch to this mailing list
containing this flag. You would add something like:

G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
g_dbus_emit_property_changed() would check this flag, in which case
process_changes() would be called synchronously. I don't see why this
wouldn't solve your problem and it doesn't require an API change. For
the exceptional cases in which this is needed, we add such flag. And
if a property needs to be sent immediately in one place I think it
always will. Or am I missing something?

I can send the patch again if you want.

Lucas De Marchi

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Vinicius Costa Gomes]

Hi Lucas,

On 14:33 Wed 24 Apr, Lucas De Marchi wrote:
> On Wed, Apr 24, 2013 at 10:58 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi,
> >
> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
> >> > What about creating a new flag for properties that need to emit a
> >> > signal immediately when they change?
> >>
> >> That would also solve the problem.
> >>
> >> The way I see it, we should still not make any guarantees about when
> >> a propery changed signal will be emitted, only when the API behaviour
> >> requires, we use some kind of barrier to keep the ordering in check.
> >>
> >> And having a function to call we make it easier to detect when this
> >> is abused, i.e. we know there's something very wrong if there's a flush
> >> after each emit.
> >
> > Is this where the discussion got stuck? Since this feature is really
> > only needed for quite exceptional situations if an API change needs to
> > be done I'd be in favor of a separate flush function.
> >
> > Another option is to make *all* D-Bus messages that gdbus sends to use
> > an idle callback, including method calls and returns. In the idle
> > callback the messages would be sent in the same order as they were added
> > to the send queue. This would not require any changes to the gdbus API.
> 
> 
> ugh.. no. This doesn't play well with sending the message directly
> through libdbus.
> 
> When we implemented the properties interface, the manner we talked
> about doing this (if it was indeed needed) was mostly what Luiz is
> saying now. I think I even sent a patch to this mailing list
> containing this flag. You would add something like:
> 
> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
> g_dbus_emit_property_changed() would check this flag, in which case
> process_changes() would be called synchronously. I don't see why this
> wouldn't solve your problem and it doesn't require an API change. For
> the exceptional cases in which this is needed, we add such flag. And
> if a property needs to be sent immediately in one place I think it
> always will. Or am I missing something?

I understand the problem a little differently. To solve this problem in the
general sense, I want the remote application to have an updated view of the
device object before receiving the NewConnection() call.

Having some properties (Paired and UUIDs) marked as immediate would only solve
this particular problem.

But it could be that I am blowing this out of proportion.

> 
> I can send the patch again if you want.
> 
> Lucas De Marchi


Cheers,
-- 
Vinicius

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Lucas De Marchi]

On Wed, Apr 24, 2013 at 2:56 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> Hi Lucas,
>
> On 14:33 Wed 24 Apr, Lucas De Marchi wrote:
>> On Wed, Apr 24, 2013 at 10:58 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> > Hi,
>> >
>> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
>> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
>> >> > What about creating a new flag for properties that need to emit a
>> >> > signal immediately when they change?
>> >>
>> >> That would also solve the problem.
>> >>
>> >> The way I see it, we should still not make any guarantees about when
>> >> a propery changed signal will be emitted, only when the API behaviour
>> >> requires, we use some kind of barrier to keep the ordering in check.
>> >>
>> >> And having a function to call we make it easier to detect when this
>> >> is abused, i.e. we know there's something very wrong if there's a flush
>> >> after each emit.
>> >
>> > Is this where the discussion got stuck? Since this feature is really
>> > only needed for quite exceptional situations if an API change needs to
>> > be done I'd be in favor of a separate flush function.
>> >
>> > Another option is to make *all* D-Bus messages that gdbus sends to use
>> > an idle callback, including method calls and returns. In the idle
>> > callback the messages would be sent in the same order as they were added
>> > to the send queue. This would not require any changes to the gdbus API.
>>
>>
>> ugh.. no. This doesn't play well with sending the message directly
>> through libdbus.
>>
>> When we implemented the properties interface, the manner we talked
>> about doing this (if it was indeed needed) was mostly what Luiz is
>> saying now. I think I even sent a patch to this mailing list
>> containing this flag. You would add something like:
>>
>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
>> g_dbus_emit_property_changed() would check this flag, in which case
>> process_changes() would be called synchronously. I don't see why this
>> wouldn't solve your problem and it doesn't require an API change. For
>> the exceptional cases in which this is needed, we add such flag. And
>> if a property needs to be sent immediately in one place I think it
>> always will. Or am I missing something?
>
> I understand the problem a little differently. To solve this problem in the
> general sense, I want the remote application to have an updated view of the
> device object before receiving the NewConnection() call.
>
> Having some properties (Paired and UUIDs) marked as immediate would only solve
> this particular problem.

if "Paired" is marked as IMMEDIATE, this would work fine:

g_dbus_emit_property_changed(conn, path, iface, "Paired");
dbus_message_send_with_reply(conn, msg, &pending, -1);


All properties set until now would be sent, and then the call to
NewConnection().

Lucas De Marchi

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Vinicius Costa Gomes]

Hi Lucas,

On 15:09 Wed 24 Apr, Lucas De Marchi wrote:

[snip]


> 
> if "Paired" is marked as IMMEDIATE, this would work fine:
> 
> g_dbus_emit_property_changed(conn, path, iface, "Paired");
> dbus_message_send_with_reply(conn, msg, &pending, -1);
> 
> 
> All properties set until now would be sent, and then the call to
> NewConnection().

Ah, I see. I was thinking that only that property would be send at that point,
if all the pending properties are sent, I am fine with this.

> 
> Lucas De Marchi

Cheers,
-- 
Vinicius

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Johan Hedberg]

Hi Lucas,

On Wed, Apr 24, 2013, Lucas De Marchi wrote:
> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
> >> > What about creating a new flag for properties that need to emit a
> >> > signal immediately when they change?
> >>
> >> That would also solve the problem.
> >>
> >> The way I see it, we should still not make any guarantees about when
> >> a propery changed signal will be emitted, only when the API behaviour
> >> requires, we use some kind of barrier to keep the ordering in check.
> >>
> >> And having a function to call we make it easier to detect when this
> >> is abused, i.e. we know there's something very wrong if there's a flush
> >> after each emit.
> >
> > Is this where the discussion got stuck? Since this feature is really
> > only needed for quite exceptional situations if an API change needs to
> > be done I'd be in favor of a separate flush function.
> >
> > Another option is to make *all* D-Bus messages that gdbus sends to use
> > an idle callback, including method calls and returns. In the idle
> > callback the messages would be sent in the same order as they were added
> > to the send queue. This would not require any changes to the gdbus API.
> 
> ugh.. no. This doesn't play well with sending the message directly
> through libdbus.

Is that something we really want to explicitly support instead of
encouraging code to always use gdbus functions?

> When we implemented the properties interface, the manner we talked
> about doing this (if it was indeed needed) was mostly what Luiz is
> saying now. I think I even sent a patch to this mailing list
> containing this flag. You would add something like:
> 
> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
> g_dbus_emit_property_changed() would check this flag, in which case
> process_changes() would be called synchronously. I don't see why this
> wouldn't solve your problem and it doesn't require an API change. For
> the exceptional cases in which this is needed, we add such flag. And
> if a property needs to be sent immediately in one place I think it
> always will. Or am I missing something?

The main point I was trying to present is that it would be nice if when
you have code like:

	foo();
	bar();
	baz();

and each of those calls create a D-Bus message (property signal method
call/return, etc) that needs to be sent, that you could count on the
messages being sent in the same order that they happen in the code. This
kind of intuitiveness would be nice to have regardless of whether we're
dealing with a special case that really needs it or not.

Johan

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Lucas De Marchi]

On Wed, Apr 24, 2013 at 3:36 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Lucas,
>
> On Wed, Apr 24, 2013, Lucas De Marchi wrote:
>> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
>> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
>> >> > What about creating a new flag for properties that need to emit a
>> >> > signal immediately when they change?
>> >>
>> >> That would also solve the problem.
>> >>
>> >> The way I see it, we should still not make any guarantees about when
>> >> a propery changed signal will be emitted, only when the API behaviour
>> >> requires, we use some kind of barrier to keep the ordering in check.
>> >>
>> >> And having a function to call we make it easier to detect when this
>> >> is abused, i.e. we know there's something very wrong if there's a flush
>> >> after each emit.
>> >
>> > Is this where the discussion got stuck? Since this feature is really
>> > only needed for quite exceptional situations if an API change needs to
>> > be done I'd be in favor of a separate flush function.
>> >
>> > Another option is to make *all* D-Bus messages that gdbus sends to use
>> > an idle callback, including method calls and returns. In the idle
>> > callback the messages would be sent in the same order as they were added
>> > to the send queue. This would not require any changes to the gdbus API.
>>
>> ugh.. no. This doesn't play well with sending the message directly
>> through libdbus.
>
> Is that something we really want to explicitly support instead of
> encouraging code to always use gdbus functions?

Unless we create a complete wrapper over libdbus functions that we
need, there's no encouraging thing here. It's just needed in some
places.


>
>> When we implemented the properties interface, the manner we talked
>> about doing this (if it was indeed needed) was mostly what Luiz is
>> saying now. I think I even sent a patch to this mailing list
>> containing this flag. You would add something like:
>>
>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
>> g_dbus_emit_property_changed() would check this flag, in which case
>> process_changes() would be called synchronously. I don't see why this
>> wouldn't solve your problem and it doesn't require an API change. For
>> the exceptional cases in which this is needed, we add such flag. And
>> if a property needs to be sent immediately in one place I think it
>> always will. Or am I missing something?
>
> The main point I was trying to present is that it would be nice if when
> you have code like:
>
>         foo();
>         bar();
>         baz();
>
> and each of those calls create a D-Bus message (property signal method
> call/return, etc) that needs to be sent, that you could count on the
> messages being sent in the same order that they happen in the code. This
> kind of intuitiveness would be nice to have regardless of whether we're
> dealing with a special case that really needs it or not.

Any of the proposed solutions do this. Your solution involves not
marking the special cases, but on the other hand makes the special
cases the default and needs some more working on gdbus to cover all
the uses of dbus_connection_send_*

I'm not saying it's not a good solution, bit I'm not sure it's the
best one neither.


Lucas De Marchi

RE: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Amro Mouslli]

Hi All , 

Any straight forward instructions on how to setup Bluetooth PAN on Ubuntu
server 11.04 ? it looks like there is no clear instruction for the new bluez
versions. 
Thank you 

Amro

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org
[mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Lucas De Marchi
Sent: Wednesday, April 24, 2013 12:02 PM
To: Lucas De Marchi; Vinicius Costa Gomes; Luiz Augusto von Dentz;
linux-bluetooth@vger.kernel.org
Subject: Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

On Wed, Apr 24, 2013 at 3:36 PM, Johan Hedberg <johan.hedberg@gmail.com>
wrote:
> Hi Lucas,
>
> On Wed, Apr 24, 2013, Lucas De Marchi wrote:
>> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
>> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
>> >> > What about creating a new flag for properties that need to emit a
>> >> > signal immediately when they change?
>> >>
>> >> That would also solve the problem.
>> >>
>> >> The way I see it, we should still not make any guarantees about when
>> >> a propery changed signal will be emitted, only when the API behaviour
>> >> requires, we use some kind of barrier to keep the ordering in check.
>> >>
>> >> And having a function to call we make it easier to detect when this
>> >> is abused, i.e. we know there's something very wrong if there's a
flush
>> >> after each emit.
>> >
>> > Is this where the discussion got stuck? Since this feature is really
>> > only needed for quite exceptional situations if an API change needs to
>> > be done I'd be in favor of a separate flush function.
>> >
>> > Another option is to make *all* D-Bus messages that gdbus sends to use
>> > an idle callback, including method calls and returns. In the idle
>> > callback the messages would be sent in the same order as they were
added
>> > to the send queue. This would not require any changes to the gdbus API.
>>
>> ugh.. no. This doesn't play well with sending the message directly
>> through libdbus.
>
> Is that something we really want to explicitly support instead of
> encouraging code to always use gdbus functions?

Unless we create a complete wrapper over libdbus functions that we
need, there's no encouraging thing here. It's just needed in some
places.


>
>> When we implemented the properties interface, the manner we talked
>> about doing this (if it was indeed needed) was mostly what Luiz is
>> saying now. I think I even sent a patch to this mailing list
>> containing this flag. You would add something like:
>>
>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
>> g_dbus_emit_property_changed() would check this flag, in which case
>> process_changes() would be called synchronously. I don't see why this
>> wouldn't solve your problem and it doesn't require an API change. For
>> the exceptional cases in which this is needed, we add such flag. And
>> if a property needs to be sent immediately in one place I think it
>> always will. Or am I missing something?
>
> The main point I was trying to present is that it would be nice if when
> you have code like:
>
>         foo();
>         bar();
>         baz();
>
> and each of those calls create a D-Bus message (property signal method
> call/return, etc) that needs to be sent, that you could count on the
> messages being sent in the same order that they happen in the code. This
> kind of intuitiveness would be nice to have regardless of whether we're
> dealing with a special case that really needs it or not.

Any of the proposed solutions do this. Your solution involves not
marking the special cases, but on the other hand makes the special
cases the default and needs some more working on gdbus to cover all
the uses of dbus_connection_send_*

I'm not saying it's not a good solution, bit I'm not sure it's the
best one neither.


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[João Paulo Rechi Vita]

Hello Guys,

On Wed, Apr 24, 2013 at 4:01 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> On Wed, Apr 24, 2013 at 3:36 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> Hi Lucas,
>>
>> On Wed, Apr 24, 2013, Lucas De Marchi wrote:
>>> > On Fri, Apr 19, 2013, Vinicius Costa Gomes wrote:
>>> >> On 09:30 Fri 19 Apr, Luiz Augusto von Dentz wrote:
>>> >> > What about creating a new flag for properties that need to emit a
>>> >> > signal immediately when they change?
>>> >>
>>> >> That would also solve the problem.
>>> >>
>>> >> The way I see it, we should still not make any guarantees about when
>>> >> a propery changed signal will be emitted, only when the API behaviour
>>> >> requires, we use some kind of barrier to keep the ordering in check.
>>> >>
>>> >> And having a function to call we make it easier to detect when this
>>> >> is abused, i.e. we know there's something very wrong if there's a flush
>>> >> after each emit.
>>> >
>>> > Is this where the discussion got stuck? Since this feature is really
>>> > only needed for quite exceptional situations if an API change needs to
>>> > be done I'd be in favor of a separate flush function.
>>> >
>>> > Another option is to make *all* D-Bus messages that gdbus sends to use
>>> > an idle callback, including method calls and returns. In the idle
>>> > callback the messages would be sent in the same order as they were added
>>> > to the send queue. This would not require any changes to the gdbus API.
>>>
>>> ugh.. no. This doesn't play well with sending the message directly
>>> through libdbus.
>>
>> Is that something we really want to explicitly support instead of
>> encouraging code to always use gdbus functions?
>
> Unless we create a complete wrapper over libdbus functions that we
> need, there's no encouraging thing here. It's just needed in some
> places.
>
>
>>
>>> When we implemented the properties interface, the manner we talked
>>> about doing this (if it was indeed needed) was mostly what Luiz is
>>> saying now. I think I even sent a patch to this mailing list
>>> containing this flag. You would add something like:
>>>
>>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
>>> g_dbus_emit_property_changed() would check this flag, in which case
>>> process_changes() would be called synchronously. I don't see why this
>>> wouldn't solve your problem and it doesn't require an API change. For
>>> the exceptional cases in which this is needed, we add such flag. And
>>> if a property needs to be sent immediately in one place I think it
>>> always will. Or am I missing something?
>>
>> The main point I was trying to present is that it would be nice if when
>> you have code like:
>>
>>         foo();
>>         bar();
>>         baz();
>>
>> and each of those calls create a D-Bus message (property signal method
>> call/return, etc) that needs to be sent, that you could count on the
>> messages being sent in the same order that they happen in the code. This
>> kind of intuitiveness would be nice to have regardless of whether we're
>> dealing with a special case that really needs it or not.
>
> Any of the proposed solutions do this. Your solution involves not
> marking the special cases, but on the other hand makes the special
> cases the default and needs some more working on gdbus to cover all
> the uses of dbus_connection_send_*
>
> I'm not saying it's not a good solution, bit I'm not sure it's the
> best one neither.
>
>

Can we get back to this topic? This problem is still present in the
master branch and affecting our support for HFP in oFono.

--
João Paulo Rechi Vita
http://about.me/jprvita

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Johan Hedberg]

Hi João Paulo,

On Wed, Jun 26, 2013, João Paulo Rechi Vita wrote:
> >>> When we implemented the properties interface, the manner we talked
> >>> about doing this (if it was indeed needed) was mostly what Luiz is
> >>> saying now. I think I even sent a patch to this mailing list
> >>> containing this flag. You would add something like:
> >>>
> >>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
> >>> g_dbus_emit_property_changed() would check this flag, in which case
> >>> process_changes() would be called synchronously. I don't see why this
> >>> wouldn't solve your problem and it doesn't require an API change. For
> >>> the exceptional cases in which this is needed, we add such flag. And
> >>> if a property needs to be sent immediately in one place I think it
> >>> always will. Or am I missing something?
> >>
> >> The main point I was trying to present is that it would be nice if when
> >> you have code like:
> >>
> >>         foo();
> >>         bar();
> >>         baz();
> >>
> >> and each of those calls create a D-Bus message (property signal method
> >> call/return, etc) that needs to be sent, that you could count on the
> >> messages being sent in the same order that they happen in the code. This
> >> kind of intuitiveness would be nice to have regardless of whether we're
> >> dealing with a special case that really needs it or not.
> >
> > Any of the proposed solutions do this. Your solution involves not
> > marking the special cases, but on the other hand makes the special
> > cases the default and needs some more working on gdbus to cover all
> > the uses of dbus_connection_send_*
> >
> > I'm not saying it's not a good solution, bit I'm not sure it's the
> > best one neither.
> 
> Can we get back to this topic? This problem is still present in the
> master branch and affecting our support for HFP in oFono.

The last chat I had with Marcel about this was that he also preferred to
have a method call sending gdbus API that guarantees the ordering of
messages (including method calls, replies and signals). We don't have to
convert absolutely everything immediately to use the API but just the
critical places where the order is actually causing bad behavior right
now.

Technical detail-wise this is not completely trivial since the pending
signal handling is part of the gdbus context data for the local object
path that emits them, whereas method calls do not have any associated
local object path (they're tied to a *remote* object path), i.e. the
same gdbus context data can't be used. One option would be to track
pending method calls in the context data for the root path which is
available as the static "root" variable in gdbus/object.c. Another
technical detail that needs deciding is whether sending a method call
should cause a forceful sending of any pending signals, or whether the
call should also be put behind an idle callback.

Johan

Re: [RFC 1/2] gdbus: Add g_dbus_flush_properties()

[Lucas De Marchi]

On Wed, Jun 26, 2013 at 4:49 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi João Paulo,
>
> On Wed, Jun 26, 2013, João Paulo Rechi Vita wrote:
>> >>> When we implemented the properties interface, the manner we talked
>> >>> about doing this (if it was indeed needed) was mostly what Luiz is
>> >>> saying now. I think I even sent a patch to this mailing list
>> >>> containing this flag. You would add something like:
>> >>>
>> >>> G_DBUS_PROPERTY_FLAG_IMMEDIATE (or some better naming). Then
>> >>> g_dbus_emit_property_changed() would check this flag, in which case
>> >>> process_changes() would be called synchronously. I don't see why this
>> >>> wouldn't solve your problem and it doesn't require an API change. For
>> >>> the exceptional cases in which this is needed, we add such flag. And
>> >>> if a property needs to be sent immediately in one place I think it
>> >>> always will. Or am I missing something?
>> >>
>> >> The main point I was trying to present is that it would be nice if when
>> >> you have code like:
>> >>
>> >>         foo();
>> >>         bar();
>> >>         baz();
>> >>
>> >> and each of those calls create a D-Bus message (property signal method
>> >> call/return, etc) that needs to be sent, that you could count on the
>> >> messages being sent in the same order that they happen in the code. This
>> >> kind of intuitiveness would be nice to have regardless of whether we're
>> >> dealing with a special case that really needs it or not.
>> >
>> > Any of the proposed solutions do this. Your solution involves not
>> > marking the special cases, but on the other hand makes the special
>> > cases the default and needs some more working on gdbus to cover all
>> > the uses of dbus_connection_send_*
>> >
>> > I'm not saying it's not a good solution, bit I'm not sure it's the
>> > best one neither.
>>
>> Can we get back to this topic? This problem is still present in the
>> master branch and affecting our support for HFP in oFono.
>
> The last chat I had with Marcel about this was that he also preferred to
> have a method call sending gdbus API that guarantees the ordering of
> messages (including method calls, replies and signals). We don't have to
> convert absolutely everything immediately to use the API but just the
> critical places where the order is actually causing bad behavior right
> now.
>
> Technical detail-wise this is not completely trivial since the pending
> signal handling is part of the gdbus context data for the local object
> path that emits them, whereas method calls do not have any associated
> local object path (they're tied to a *remote* object path), i.e. the
> same gdbus context data can't be used. One option would be to track
> pending method calls in the context data for the root path which is
> available as the static "root" variable in gdbus/object.c. Another
> technical detail that needs deciding is whether sending a method call
> should cause a forceful sending of any pending signals, or whether the
> call should also be put behind an idle callback.

If you really want to do this (which I don't completely agree with), I
think the only option is to send pending signals when there's a
method-{reply,call}. And delay sending only the signals that do need
to be delayed. The only one I remember off is the
org.freedesktop.DBus.PropertiesChanged() signal.  We delay sending
this signal to an idler exactly because we want to group all pending
properties into only 1 signal. But if in the meantime there's another
d-bus message, if you want to guarantee the order you need to "flush
pending property changes". All the other signals, method returns and
calls can be sent right away. Putting a method call behind an idler
doesn't really solve the problem since it doesn't guarantee the order.
E.g.:

1) g_dbus_property_changed(A);
2) g_dbus_property_changed(B);
3) g_dbus_send_method_call(...);
4) g_dbus_property_changed(C);

Because of the fact that we group property changes, it doesn't matter
how you treat the pending messages in the idler, property C will be
sent together with A and B, either before or after the method call,
thus not guaranteeing the order.

On the other hand, if you make method calls and replies to force
pending signals to be sent, in the case above you will have the
messages as below and this doesn't need the idler thing.

=> signal org.freedesktop.DBus.PropertiesChanged(A, B)
=> method-call
=> signal org.freedesktop.DBus.PropertiesChanged(C)

And if you reorder 3 and 4 it also works:

=> signal org.freedesktop.DBus.PropertiesChanged(A, B, C)
=> method-call

This is the first problem to solve. The second one, and more tricky is
regarding property changes on different interfaces/objects. Should we
also guarantee the order? E.g:

1) g_dbus_property_changed(obj, ifaceA, "Bla");
2) g_dbus_property_changed(obj, ifaceB, "Bla");

Right now the order is not guaranteed and depend on the object path
and the order in which the interfaces were registered. I think this is
not really important and it's one thing we should never give
guarantees API-wise, so IMO it can stays as such.

The third problem is about how we coalesce property changes. E.g:

obj->state = "a";
g_dbus_property_changed("State");
obj->state = "b";
g_dbus_property_changed("State");

The user ends up never seeing State="a" because we only get the value
in an idler, when it already changed. I think one user that would need
this to be changed is oFono, that may need to report specific
transitions in voice call states. However this is only one example,
I'm not sure if oFono is planning to change its interface. I  think we
can change this by start constructing the signal right when the
property changes. AFAIR we do what we do now because of the libdbus
API, that don't let we append properties in the changed_properties
dict after we have already started appending properties to the
invalidated_properties array.  Without changing libdbus (or using
another library like ell/systemd-bus*) what we can do is to force
sealing the message when there's an invalidated property to be sent,
or....  if there's a changed property to be sent, but pending
invalidated properties already.

Lucas De Marchi

* I'm not sure if they provide such a thing though.