From 3f8f7c0983f2a081e194ca3620500ed023bb9088 Mon Sep 17 00:00:00 2001 From: David Zeuthen Date: Fri, 19 Jun 2009 18:26:01 -0400 Subject: [PATCH] Sort out dbus-glib registration mess We sometimes segfaulted when stopping a RAID device. The problem here was that we kept the exported D-Bus object around until it was finalized (by using weak refs). This was problematic because several change events were treated as add events but the state of the RAID device was such that we didn't want to add the object anyway. So we ended up unreffing the object. Which means that the job completion func accessed a unreferenced object. The solution here is to remove the object from our hashes as soon as we want to remove it. Since we don't have dbus_g_connection_unregister_g_object() just yet (available in dbus-glib >= 0.81) we just keep the object on the bus. If we later wants to add a new object but the old object is still around we use an undocumented hack to unregister the object. You will also need eggdbus >= 0.5, otherwise introspection won't work. --- src/devkit-disks-daemon.c | 104 +++++++++++--------------------------- src/devkit-disks-device-private.c | 13 +++-- src/devkit-disks-device-private.h | 1 - src/devkit-disks-device.c | 88 ++++++++++++++++++++------------ 4 files changed, 94 insertions(+), 112 deletions(-) diff --git a/src/devkit-disks-daemon.c b/src/devkit-disks-daemon.c index 593dfc5..ac098f2 100644 --- a/src/devkit-disks-daemon.c +++ b/src/devkit-disks-daemon.c @@ -527,19 +527,19 @@ devkit_disks_daemon_init (DevkitDisksDaemon *daemon) daemon->priv->map_dev_t_to_device = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, - NULL); + g_object_unref); daemon->priv->map_device_file_to_device = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, - NULL); + g_object_unref); daemon->priv->map_native_path_to_device = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, - NULL); + g_object_unref); daemon->priv->map_object_path_to_device = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, - NULL); + g_object_unref); } static void @@ -638,44 +638,6 @@ _filter (DBusConnection *connection, DBusMessage *message, void *user_data) return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } -static gboolean -device_went_away_remove_quiet_cb (gpointer key, gpointer value, gpointer user_data) -{ - if (value == user_data) { - return TRUE; - } - return FALSE; -} - -static gboolean -device_went_away_remove_cb (gpointer key, gpointer value, gpointer user_data) -{ - if (value == user_data) { - g_print ("**** REMOVED %s\n", (char *) key); - return TRUE; - } - return FALSE; -} - -static void -device_went_away (gpointer user_data, GObject *where_the_object_was) -{ - DevkitDisksDaemon *daemon = DEVKIT_DISKS_DAEMON (user_data); - - g_hash_table_foreach_remove (daemon->priv->map_device_file_to_device, - device_went_away_remove_cb, - where_the_object_was); - g_hash_table_foreach_remove (daemon->priv->map_dev_t_to_device, - device_went_away_remove_quiet_cb, - where_the_object_was); - g_hash_table_foreach_remove (daemon->priv->map_native_path_to_device, - device_went_away_remove_quiet_cb, - where_the_object_was); - g_hash_table_foreach_remove (daemon->priv->map_object_path_to_device, - device_went_away_remove_quiet_cb, - where_the_object_was); -} - static void device_add (DevkitDisksDaemon *daemon, GUdevDevice *d, gboolean emit_event); static void device_remove (DevkitDisksDaemon *daemon, GUdevDevice *d); @@ -740,27 +702,23 @@ device_add (DevkitDisksDaemon *daemon, GUdevDevice *d, gboolean emit_event) device = devkit_disks_device_new (daemon, d); if (device != NULL) { - /* only take a weak ref; the device will stay on the bus until - * it's unreffed. So if we ref it, it'll never go away. Stupid - * dbus-glib, no cookie for you. - */ - g_object_weak_ref (G_OBJECT (device), device_went_away, daemon); g_hash_table_insert (daemon->priv->map_dev_t_to_device, GINT_TO_POINTER (devkit_disks_device_local_get_dev (device)), - device); + g_object_ref (device)); g_hash_table_insert (daemon->priv->map_device_file_to_device, g_strdup (devkit_disks_device_local_get_device_file (device)), - device); + g_object_ref (device)); g_hash_table_insert (daemon->priv->map_native_path_to_device, g_strdup (native_path), - device); + g_object_ref (device)); g_hash_table_insert (daemon->priv->map_object_path_to_device, g_strdup (devkit_disks_device_local_get_object_path (device)), - device); + g_object_ref (device)); g_print ("**** ADDED %s\n", native_path); if (emit_event) { const char *object_path; object_path = devkit_disks_device_local_get_object_path (device); + g_print ("**** EMITTING ADDED for %s\n", device->priv->native_path); g_signal_emit (daemon, signals[DEVICE_ADDED_SIGNAL], 0, object_path); } devkit_disks_daemon_local_update_poller (daemon); @@ -782,10 +740,26 @@ device_remove (DevkitDisksDaemon *daemon, GUdevDevice *d) g_print ("**** IGNORING REMOVE %s\n", native_path); } else { g_print ("**** REMOVING %s\n", native_path); + + g_warn_if_fail (g_strcmp0 (native_path, device->priv->native_path) == 0); + + g_warn_if_fail (g_hash_table_remove (daemon->priv->map_native_path_to_device, + device->priv->native_path)); + g_warn_if_fail (g_hash_table_remove (daemon->priv->map_device_file_to_device, + device->priv->device_file)); + g_warn_if_fail (g_hash_table_remove (daemon->priv->map_object_path_to_device, + device->priv->object_path)); + g_warn_if_fail (g_hash_table_remove (daemon->priv->map_dev_t_to_device, + GINT_TO_POINTER (device->priv->dev))); + devkit_disks_device_removed (device); + + g_print ("**** EMITTING REMOVED for %s\n", device->priv->native_path); g_signal_emit (daemon, signals[DEVICE_REMOVED_SIGNAL], 0, devkit_disks_device_local_get_object_path (device)); + g_object_unref (device); + devkit_disks_daemon_local_update_poller (daemon); } } @@ -805,7 +779,7 @@ on_uevent (GUdevClient *client, } else if (strcmp (action, "change") == 0) { device_changed (daemon, device, FALSE); } else { - g_warning ("unhandled action '%s' on %s", action, g_udev_device_get_sysfs_path (device)); + g_print ("*** NOTE: unhandled action '%s' on %s\n", action, g_udev_device_get_sysfs_path (device)); } } @@ -993,16 +967,9 @@ register_disks_daemon (DevkitDisksDaemon *daemon) G_OBJECT (daemon)); daemon->priv->system_bus_proxy = dbus_g_proxy_new_for_name (daemon->priv->system_bus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS); - - /* TODO FIXME: I'm pretty sure dbus-glib blows in a way that - * we can't say we're interested in all signals from all - * members on all interfaces for a given service... So we do - * this.. - */ - + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS); dbus_error_init (&dbus_error); /* need to listen to NameOwnerChanged */ @@ -1019,17 +986,6 @@ register_disks_daemon (DevkitDisksDaemon *daemon) goto error; } - /* need to listen to ConsoleKit signals */ - dbus_bus_add_match (connection, - "type='signal',sender='org.freedesktop.ConsoleKit'", - &dbus_error); - - if (dbus_error_is_set (&dbus_error)) { - g_warning ("Cannot add match rule: %s: %s", dbus_error.name, dbus_error.message); - dbus_error_free (&dbus_error); - goto error; - } - if (!dbus_connection_add_filter (connection, _filter, daemon, @@ -1055,7 +1011,7 @@ register_disks_daemon (DevkitDisksDaemon *daemon) error = NULL; } - /* connect to the DeviceKit daemon */ + /* connect to udev */ daemon->priv->gudev_client = g_udev_client_new (subsystems); g_signal_connect (daemon->priv->gudev_client, "uevent", diff --git a/src/devkit-disks-device-private.c b/src/devkit-disks-device-private.c index bf25e3e..d78c1b2 100644 --- a/src/devkit-disks-device-private.c +++ b/src/devkit-disks-device-private.c @@ -32,11 +32,14 @@ emit_changed_idle_cb (gpointer data) //g_debug ("XXX emitting 'changed' in idle"); - g_signal_emit_by_name (device->priv->daemon, - "device-changed", - device->priv->object_path); - g_signal_emit_by_name (device, "changed"); - + if (!device->priv->removed) + { + g_print ("**** EMITTING CHANGED for %s\n", device->priv->native_path); + g_signal_emit_by_name (device->priv->daemon, + "device-changed", + device->priv->object_path); + g_signal_emit_by_name (device, "changed"); + } device->priv->emit_changed_idle_id = 0; /* remove the idle source */ diff --git a/src/devkit-disks-device-private.h b/src/devkit-disks-device-private.h index 1ba400e..acfdfbf 100644 --- a/src/devkit-disks-device-private.h +++ b/src/devkit-disks-device-private.h @@ -64,7 +64,6 @@ typedef struct Job Job; struct DevkitDisksDevicePrivate { DBusGConnection *system_bus_connection; - DBusGProxy *system_bus_proxy; DevkitDisksDaemon *daemon; GUdevDevice *d; diff --git a/src/devkit-disks-device.c b/src/devkit-disks-device.c index 814c533..6c787a6 100644 --- a/src/devkit-disks-device.c +++ b/src/devkit-disks-device.c @@ -1209,6 +1209,8 @@ devkit_disks_device_finalize (GObject *object) device = DEVKIT_DISKS_DEVICE (object); g_return_if_fail (device->priv != NULL); + g_debug ("finalizing %s", device->priv->native_path); + g_object_unref (device->priv->d); g_object_unref (device->priv->daemon); g_free (device->priv->object_path); @@ -1349,15 +1351,24 @@ register_disks_device (DevkitDisksDevice *device) device->priv->object_path = compute_object_path (device->priv->native_path); + if (dbus_g_connection_lookup_g_object (device->priv->system_bus_connection, + device->priv->object_path) != NULL) { + /* TODO: see devkit_disks_device_removed() for where we want to unregister the object but + * we're missing the API. So do it manually here if we are forced to do so... + */ + + g_print ("**** HACK: Wanting to register object at path `%s' but there is already an " + "object there. Using a hack to move it out of the way.\n", + device->priv->object_path); + + dbus_connection_unregister_object_path (dbus_g_connection_get_connection (device->priv->system_bus_connection), + device->priv->object_path); + } + dbus_g_connection_register_g_object (device->priv->system_bus_connection, device->priv->object_path, G_OBJECT (device)); - device->priv->system_bus_proxy = dbus_g_proxy_new_for_name (device->priv->system_bus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS); - return TRUE; error: @@ -1479,7 +1490,8 @@ _dupv8 (const char *s) if (!g_utf8_validate (s, -1, &end_valid)) { - g_warning ("The string '%s' is not valid UTF-8. Invalid characters begins at '%s'", s, end_valid); + g_print ("**** NOTE: The string '%s' is not valid UTF-8. Invalid characters begins at '%s'\n", + s, end_valid); return g_strndup (s, end_valid - s); } else { return g_strdup (s); @@ -1543,7 +1555,7 @@ decode_udev_encoded_string (const gchar *str) gint val; if (str[n + 1] != 'x' || str[n + 2] == '\0' || str[n + 3] == '\0') { - g_warning ("malformed encoded string %s", str); + g_print ("**** NOTE: malformed encoded string '%s'\n", str); break; } @@ -1558,7 +1570,8 @@ decode_udev_encoded_string (const gchar *str) } if (!g_utf8_validate (s->str, -1, &end_valid)) { - g_warning ("The string '%s' is not valid UTF-8. Invalid characters begins at '%s'", s->str, end_valid); + g_print ("**** NOTE: The string '%s' is not valid UTF-8. Invalid characters begins at '%s'\n", + s->str, end_valid); ret = g_strndup (s->str, end_valid - s->str); g_string_free (s, TRUE); } else { @@ -2375,14 +2388,14 @@ update_info_linux_md (DevkitDisksDevice *device) /* figure out if the array is active */ array_state = sysfs_get_string (device->priv->native_path, "md/array_state"); if (array_state == NULL) { - g_debug ("Linux MD array %s has no array_state file'; removing", device->priv->native_path); + g_print ("**** NOTE: Linux MD array %s has no array_state file'; removing\n", device->priv->native_path); goto out; } g_strstrip (array_state); /* ignore clear arrays since these have no devices, no size, no level */ if (strcmp (array_state, "clear") == 0) { - g_debug ("Linux MD array %s is 'clear'; removing", device->priv->native_path); + g_print ("**** NOTE: Linux MD array %s is 'clear'; removing\n", device->priv->native_path); g_free (array_state); goto out; } @@ -2522,7 +2535,9 @@ update_info_linux_md (DevkitDisksDevice *device) devkit_disks_device_set_linux_md_sync_percentage (device, 100.0 * ((double) done) / ((double) remaining)); } else { - g_debug ("cannot parse md/sync_completed: '%s'", s); + g_warning ("cannot parse md/sync_completed for %s: '%s'", + device->priv->native_path, + s); } g_free (s); @@ -3208,7 +3223,7 @@ out: if (device2 != NULL) { update_info (device2); } else { - g_warning ("### %s added non-existant slave %s", device->priv->object_path, objpath2); + g_print ("**** NOTE: %s added non-existant slave %s\n", device->priv->object_path, objpath2); } } for (l = removed_objpath; l != NULL; l = l->next) { @@ -3238,7 +3253,7 @@ out: if (device2 != NULL) { update_info (device2); } else { - g_warning ("### %s added non-existant holder %s", device->priv->object_path, objpath2); + g_print ("**** NOTE: %s added non-existant holder %s\n", device->priv->object_path, objpath2); } } for (l = removed_objpath; l != NULL; l = l->next) { @@ -3414,6 +3429,12 @@ devkit_disks_device_removed (DevkitDisksDevice *device) device->priv->removed = TRUE; + /* TODO: this is in a yet to be released version of dbus-glib, use it when available + + dbus_g_connection_unregister_g_object (device->priv->system_bus_connection, + G_OBJECT (device)); + */ + /* device is now removed; update all slaves and holders */ for (n = 0; n < device->priv->slaves_objpath->len; n++) { const gchar *objpath2 = ((gchar **) device->priv->slaves_objpath->pdata)[n]; @@ -3516,8 +3537,9 @@ drain_pending_changes (DevkitDisksDevice *device, gboolean force_update) emit_changed = TRUE; } - if (emit_changed || force_update) { + if ((!device->priv->removed) && (emit_changed || force_update)) { if (device->priv->object_path != NULL) { + g_print ("**** EMITTING CHANGED for %s\n", device->priv->native_path); g_signal_emit_by_name (device, "changed"); g_signal_emit_by_name (device->priv->daemon, "device-changed", device->priv->object_path); } @@ -3529,22 +3551,24 @@ emit_job_changed (DevkitDisksDevice *device) { drain_pending_changes (device, FALSE); - g_print ("emitting job-changed on %s\n", device->priv->native_path); - g_signal_emit_by_name (device->priv->daemon, - "device-job-changed", - device->priv->object_path, + if (!device->priv->removed) { + g_print ("**** EMITTING JOB-CHANGED for %s\n", device->priv->native_path); + g_signal_emit_by_name (device->priv->daemon, + "device-job-changed", + device->priv->object_path, + device->priv->job_in_progress, + device->priv->job_id, + device->priv->job_initiated_by_uid, + device->priv->job_is_cancellable, + device->priv->job_percentage, + NULL); + g_signal_emit (device, signals[JOB_CHANGED_SIGNAL], 0, device->priv->job_in_progress, device->priv->job_id, device->priv->job_initiated_by_uid, device->priv->job_is_cancellable, - device->priv->job_percentage, - NULL); - g_signal_emit (device, signals[JOB_CHANGED_SIGNAL], 0, - device->priv->job_in_progress, - device->priv->job_id, - device->priv->job_initiated_by_uid, - device->priv->job_is_cancellable, - device->priv->job_percentage); + device->priv->job_percentage); + } } /* called by the daemon on the 'change' uevent */ @@ -9069,14 +9093,14 @@ force_unmount_completed_cb (DBusGMethodInvocation *context, if (WEXITSTATUS (status) == 0 && !job_was_cancelled) { - g_debug ("Successfully force unmounted device %s", device->priv->device_file); + g_print ("**** NOTE: Successfully force unmounted device %s\n", device->priv->device_file); /* update_info_mount_state() will update the mounts file and clean up the directory if needed */ update_info (device); if (data->fr_callback != NULL) data->fr_callback (device, TRUE, data->fr_user_data); } else { - g_debug ("force unmount failed: %s", stderr); + g_print ("**** NOTE: force unmount failed: %s\n", stderr); if (data->fr_callback != NULL) data->fr_callback (device, FALSE, data->fr_user_data); } @@ -9140,12 +9164,12 @@ force_luks_teardown_completed_cb (DBusGMethodInvocation *context, if (WEXITSTATUS (status) == 0 && !job_was_cancelled) { - g_debug ("Successfully teared down luks device %s", device->priv->device_file); + g_print ("**** NOTE: Successfully teared down luks device %s\n", device->priv->device_file); if (data->fr_callback != NULL) data->fr_callback (device, TRUE, data->fr_user_data); } else { - g_warning ("force luks teardown failed: %s", stderr); + g_print ("**** NOTE: force luks teardown failed: %s\n", stderr); if (data->fr_callback != NULL) data->fr_callback (device, FALSE, data->fr_user_data); } @@ -9262,7 +9286,7 @@ force_removal (DevkitDisksDevice *device, gboolean remove_dir_on_unmount; if (devkit_disks_mount_file_has_device (device->priv->device_file, NULL, &remove_dir_on_unmount)) { - g_debug ("Force unmounting device %s", device->priv->device_file); + g_print ("**** NOTE: Force unmounting device %s\n", device->priv->device_file); force_unmount (device, callback, user_data); goto pending; } @@ -9284,7 +9308,7 @@ force_removal (DevkitDisksDevice *device, if (d->priv->dm_name != NULL && g_str_has_prefix (d->priv->dm_name, "devkit-disks-luks-uuid-")) { - g_debug ("Force luks teardown device %s (cleartext %s)", + g_print ("**** NOTE: Force luks teardown device %s (cleartext %s)\n", device->priv->device_file, d->priv->device_file); -- 2.7.4