[pulseaudio-discuss] [PATCH 29/56] bluetooth: Implement org.bluez.MediaEndpoint1.SetConfiguration()

[Tanu Kaskinen]

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).

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.

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.

The other call is in parse_interfaces_and_properties(). There the
handling should be the same: fail for -1, and succeed for 0.

> 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.

-- 
Tanu