On Sat, Aug 3, 2013 at 2:12 PM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Thu, 2013-08-01 at 21:10 -0300, Jo?o Paulo Rechi Vita wrote:
>> >> + }
>> >> +
>> >> + dbus_message_iter_next(&props);
>> >> + }
>> >> +
>> >> + d = pa_bluetooth_discovery_get_device_by_path(y, dev_path);
>> >> + if (!d) {
>> >> + pa_log_warn("SetConfiguration() received for unknown device %s", dev_path);
>> >> + d = pa_bluetooth_discovery_create_device(y, dev_path);
>> >> + if (!d)
>> >> + goto fail;
>> >> + /* If this point is reached the InterfacesAdded signal is probably on it's way */
>> >
>> > Can things really happen in this order? If they can, should the device
>> > created here be removed in the fail section?
>> >
>>
>> The point the comments references is when the if branch is not taken,
>> there is, d is not NULL, so we don't go to the fail section. If we
>> take the if branch, going to the fail section, no device has been
>> created, so there is nothing to be removed.
>
> Sorry, I expressed myself badly. My point was that there are other "goto
> fail" instances later in the function. If those are triggered, should
> the device that is created here be removed? My own opinion is, after you
> pointed out that we shouldn't remove devices if parsing their properties
> fails, that we shouldn't remove the device here either in case of
> failure. We should keep device objects around for every device that we
> ever see (until we get InterfacesRemoved).
>
While reviewing this I found out that there is no point in checking
for the return value of device_create() here, since it will either
return a valid pointer or the process will exit due to being OOM. I'm
also adding a comment about device_info_valid being set to 0 here.
> I noticed a couple of related bugs: there are two
> pa_bluetooth_discovery_get_device_by_path() calls in bluez5-util.c, and
> in both cases the code doesn't take it into account that the function
> returns a non-NULL pointer only if device_info_valid equals 1. If a
> device has a device_info_valid value of 0 or -1, we create a duplicate
> device object.
>
Both of this should use pa_hashmap_get() directly, since we know the
internals of pa_bluetooth_discovery here, and also take
device_info_valid into consideration.
> One of the calls is in the quoted code, i.e. in
> endpoint_set_configuration(). How should we handle a device_info_valid
> value of 0 or -1 in this case? I think endpoint_set_configuration()
> should fail for -1, and succeed for 0.
>
I agree.
> The other call is in parse_interfaces_and_properties(). There the
> handling should be the same: fail for -1, and succeed for 0.
>
I think in case of -1 here we can simply reset the information we have
about the device and set device_info_valid to 0. This is the same
behavior as if we had removed the device instead of setting
device_info_valid to -1 in a previous moment.
>> And because of the way the D-Bus helpers are implemented inside BlueZ,
>> I think it is possible that the SetConfiguration() call arrives before
>> the InterfacesAdded signal, since signals are always sent in an idler
>> and method calls are sent right away.
>
> In my opinion it's a bug in bluetoothd if it mentions device object
> paths before sending a corresponding InterfacesAdded signal.
>
I agree, but this not something trivial to fix. All D-Bus code in
bluetoothd sits on top of a gobject-like D-Bus abstraction called
gdbus, which is implemented as part of BlueZ (not to be confused with
any D-Bus abstraction that may be offered by glib). It has been
discussed quite a few times in linux-bluetooth how to improve that
code, since it also leads to a race condition between bluetoothd and
ofonod (which also uses gdbus, btw). So atm we should protect
ourselves from this problem in PA instead of waiting / proposing a
solution in bluetoothd.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
