Hi, The general idea of the patch looks good but there are a few things I'd still fix: On Tue, Sep 15, 2009, RISKÓ Gergely wrote: > -static DBusHandlerResult introspect(DBusConnection *connection, > - DBusMessage *message, struct generic_data *data) > +static DBusMessage *introspect(DBusConnection *connection, > + DBusMessage *message, void *data_) s/data_/user_data/ (to conform to the rest of the code and glib conventions) > + struct generic_data *data = (struct generic_data *) data_; Unnecessary explicit type-cast of a void pointer. > static struct generic_data *object_path_ref(DBusConnection *connection, > const char *path) > { > struct generic_data *data; > + struct interface_data *iface; > > if (dbus_connection_get_object_path_data(connection, path, > (void *) &data) == TRUE) { > @@ -363,12 +355,23 @@ static struct generic_data *object_path_ref(DBusConnection *connection, > > invalidate_parent_data(connection, path); > > + /* Register the introspection interface */ > + iface = g_new0(struct interface_data, 1); > + iface->name = g_strdup("org.freedesktop.DBus.Introspectable"); // TODO: macro Either add the macro or remove this comment. Also, we don't use C++ style commenting. > + iface->methods = introspect_methods; > + iface->signals = NULL; > + iface->properties = NULL; > + iface->user_data = data; > + iface->destroy = NULL; > + data->interfaces = g_slist_append(data->interfaces, iface); > + Instead of this duplicate interface initialization code I think it would make sense to refactor and create a new private function, e.g static void add_interface(struct generic_cata *data, ... and call that from the necessary places (afaics there are two of them) > + /* Free the introspection interface */ > + iface = find_interface(data->interfaces, "org.freedesktop.DBus.Introspectable"); > + if (iface) { > + data->interfaces = g_slist_remove(data->interfaces, iface); > + > + if (iface->destroy) > + iface->destroy(iface->user_data); > + > + g_free(iface->name); > + g_free(iface); Would it make sense to refactor create a remove_interface function for this too (to balance with the add_interface function)? Johan -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html