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
