Hi Tanu,
On Mon, Dec 31, 2012 at 12:47 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Wed, 2012-12-19 at 13:58 +0100, Mikel Astiz wrote:
>> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
>>
>> Consider the media transport when a PropertiesChanged signal is
>> received.
>>
>> Note that the transport might have an owner other than BlueZ, and thus
>> the property changes would be emitted from arbitrary senders. For
>> performance reasons, the installed match considers the interface name
>> where the property has changed.
>>
>> It could be possible to install and remove the D-Bus matches dynamically
>> when a new owner is registered/unregistered, but filtering based on the
>> interface name seems good enough already.
>> ---
>> src/modules/bluetooth/bluetooth-util.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
>>
>> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
>> index 3c0fcf7..f8ecb15 100644
>> --- a/src/modules/bluetooth/bluetooth-util.c
>> +++ b/src/modules/bluetooth/bluetooth-util.c
>> @@ -1087,6 +1087,25 @@ static int transport_parse_property(pa_bluetooth_transport *t, DBusMessageIter *
>> return 0;
>> }
>>
>> +static int parse_transport_properties(pa_bluetooth_transport *t, DBusMessageIter *i) {
>> + DBusMessageIter element_i;
>> +
>> + dbus_message_iter_recurse(i, &element_i);
>> +
>> + while (dbus_message_iter_get_arg_type(&element_i) == DBUS_TYPE_DICT_ENTRY) {
>> + DBusMessageIter dict_i;
>> +
>> + dbus_message_iter_recurse(&element_i, &dict_i);
>> +
>> + if (transport_parse_property(t, &dict_i) < 0)
>> + return -1;
>
> It would be better to not return here. Just store the fact that parsing
> some property failed in some boolean and continue parsing the next
> property, so that any valid property changes aren't missed (the same
> applies to parse_device_properties()).
I'd actually avoid this, see the answer below to the proposed guidelines.
>
> I think parse_transport_properties() could also be used in
> endpoint_set_configuration().
The idea was to keep the constant properties in
endpoint_set_configuration() and move the variables ones to
parse_transport_properties(). Note that many of these properties need
to be fixed ("UUID") or even just part of SetConfiguration
("Configuration").
Perhaps you're proposing to call parse_transport_properties() instead
of having duplicated code for "NREC"? There might be some other
properties of the same kind in the future.
>
> A suggestion for a general policy for property parse error handling:
>
> * Whenever parsing a property fails, print an error message to the log
> (already done, I think).
>
> * When parsing the initial properties of a new object, fail hard (i.e.
> don't create the object) if all non-optional properties (that we're
> interested in) are not received, or if parsing any such property fails.
Agreed, but left pending, as replied to the patch affecting device_info_valid.
>
> * When parsing a PropertiesChanged signal, never fail if there are
> parse errors, just pretend that the problematic properties didn't
> change.
I'm fine with that too.
>
> * When parsing any optional property, never fail on parse errors, just
> pretend that the property isn't set (when parsing the initial
> properties) or that the property value didn't change (when parsing a
> PropertiesChanged signal).
If a parsing of a property fails, regardless of whether it's optional
or not, it means BlueZ is misbehaving. I see no reason to be more
relaxed with optional properties.
Cheers,
Mikel
