[pulseaudio-discuss] [PATCH reserve 2/2] Initialize monitor's busy status to false if we own the device.

[David Henningsson]

On 01/12/2013 07:22 PM, Tanu Kaskinen wrote:
> Bug found by David Henningsson.
> ---
>   reserve-monitor.c |   74 +++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/reserve-monitor.c b/reserve-monitor.c
> index 4097a6f..39a0c77 100644
> --- a/reserve-monitor.c
> +++ b/reserve-monitor.c
> @@ -59,6 +59,22 @@ struct rm_monitor {
>   	"member='NameOwnerChanged',"		\
>   	"arg0='%s'"
>
> +static unsigned get_busy(
> +	DBusConnection *c,
> +	const char *name_owner) {
> +
> +	const char *un;
> +
> +	if (!name_owner || !*name_owner)
> +		return FALSE;
> +
> +	if ((un = dbus_bus_get_unique_name(c)))
> +		if (strcmp(name_owner, un) == 0)
> +			return FALSE;
> +
> +	return TRUE;
> +}
> +
>   static DBusHandlerResult filter_handler(
>   	DBusConnection *c,
>   	DBusMessage *s,
> @@ -87,16 +103,7 @@ static DBusHandlerResult filter_handler(
>   		if (strcmp(name, m->service_name) == 0) {
>   			unsigned old_busy = m->busy;
>
> -			m->busy = !!(new && *new);
> -
> -			/* If we ourselves own the device, then don't consider this 'busy' */

This comment got lost. Should be moved to the get_busy function

> -			if (m->busy) {
> -				const char *un;
> -
> -				if ((un = dbus_bus_get_unique_name(c)))
> -					if (strcmp(new, un) == 0)
> -						m->busy = FALSE;
> -			}
> +			m->busy = get_busy(c, new);
>
>   			if (m->busy != old_busy && m->change_cb) {
>   				m->ref++;
> @@ -115,11 +122,13 @@ invalid:
>   int rm_watch(
>   	rm_monitor **_m,
>   	DBusConnection *connection,
> -	const char*device_name,
> +	const char *device_name,
>   	rm_change_cb_t change_cb,
>   	DBusError *error)  {
>
>   	rm_monitor *m = NULL;
> +	DBusMessage *msg = NULL, *reply = NULL;
> +	const char *name_owner;
>   	int r;
>   	DBusError _error;
>
> @@ -178,13 +187,44 @@ int rm_watch(
>
>   	m->matching = 1;
>
> -	m->busy = dbus_bus_name_has_owner(m->connection, m->service_name, error);
> +	if (!(msg = dbus_message_new_method_call(DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, "GetNameOwner"))) {
> +		r = -ENOMEM;
> +		goto fail;
> +	}

The rm_watch function is long enough already - I'd prefer if you move 
the new code to a separate get_name_ower function (even if that would 
mean a strdup).

>
> -	if (dbus_error_is_set(error)) {
> -		r = -EIO;
> +	if (!dbus_message_append_args(msg, DBUS_TYPE_STRING, &m->service_name, DBUS_TYPE_INVALID)) {
> +		r = -ENOMEM;
>   		goto fail;
>   	}
>
> +	reply = dbus_connection_send_with_reply_and_block(m->connection, msg, DBUS_TIMEOUT_USE_DEFAULT, error);
> +	dbus_message_unref(msg);
> +	msg = NULL;
> +
> +	if (reply) {
> +		if (!dbus_message_get_args(reply, error, DBUS_TYPE_STRING, &name_owner, DBUS_TYPE_INVALID)) {
> +			r = -EIO;
> +			goto fail;
> +		}
> +	} else {
> +		if (dbus_error_has_name(error, "org.freedesktop.DBus.Error.NameHasNoOwner")) {
> +			dbus_error_free(error);
> +			name_owner = NULL;
> +		} else {
> +			r = -EIO;
> +			goto fail;
> +		}
> +	}
> +
> +	m->busy = get_busy(m->connection, name_owner);
> +
> +	/* The reply can't be unreffed earlier, because name_owner points
> +	 * inside the message. */
> +	if (reply) {
> +		dbus_message_unref(reply);
> +		reply = NULL;
> +	}
> +
>   	*_m = m;
>   	return 0;
>
> @@ -192,6 +232,12 @@ fail:
>   	if (&_error == error)
>   		dbus_error_free(&_error);
>
> +	if (msg)
> +		dbus_message_unref(msg);
> +
> +	if (reply)
> +		dbus_message_unref(reply);
> +
>   	if (m)
>   		rm_release(m);
>
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic