From bd45f846fc2db390d7abc3a2aa0a16c2040f1e6f Mon Sep 17 00:00:00 2001 From: Pauli Virtanen Date: Thu, 10 Nov 2022 21:25:42 +0200 Subject: [PATCH] bluez5: midi: refcounting and other correctness fixes Use stdalign.h instead of union. Fix some refcounting and return values. Fail early in add_filters. Minor style cleanups. Less magical spa_dbus_async_call. --- spa/plugins/bluez5/dbus-manager.c | 18 ++--- spa/plugins/bluez5/dbus-monitor.c | 119 ++++++++++++------------------ spa/plugins/bluez5/dbus-monitor.h | 29 +++++--- spa/plugins/bluez5/midi-enum.c | 30 ++++---- spa/plugins/bluez5/midi-node.c | 10 +-- spa/plugins/bluez5/midi-server.c | 22 +++--- 6 files changed, 105 insertions(+), 123 deletions(-) diff --git a/spa/plugins/bluez5/dbus-manager.c b/spa/plugins/bluez5/dbus-manager.c index fe79b11a5..99287b6bf 100644 --- a/spa/plugins/bluez5/dbus-manager.c +++ b/spa/plugins/bluez5/dbus-manager.c @@ -24,6 +24,7 @@ #include #include +#include #include @@ -57,11 +58,7 @@ struct impl struct object { struct impl *impl; - union { - struct spa_dbus_local_object this; - /* extra data follows 'this', force safer alignment */ - long double _align; - }; + struct spa_dbus_local_object alignas(max_align_t) this; }; const struct spa_dbus_property *object_interface_get_property(struct object *o, @@ -218,7 +215,8 @@ static DBusMessage *object_properties_set(struct object *o, DBusMessage *m) int res; if (!dbus_message_has_signature(m, "ssv")) - return NULL; + return dbus_message_new_error(m, DBUS_ERROR_INVALID_ARGS, + "Invalid arguments"); dbus_message_iter_init(m, &it); dbus_message_iter_get_basic(&it, &interface); @@ -298,10 +296,12 @@ static DBusHandlerResult object_handler(DBusConnection *c, DBusMessage *m, void return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } - if (r != NULL && dbus_connection_send(impl->conn, r, NULL)) - return DBUS_HANDLER_RESULT_HANDLED; - else if (r) + if (r != NULL && dbus_connection_send(impl->conn, r, NULL)) { dbus_message_unref(r); + return DBUS_HANDLER_RESULT_HANDLED; + } else if (r) { + dbus_message_unref(r); + } return DBUS_HANDLER_RESULT_NEED_MEMORY; } diff --git a/spa/plugins/bluez5/dbus-monitor.c b/spa/plugins/bluez5/dbus-monitor.c index 4e615f82b..6286d7b0b 100644 --- a/spa/plugins/bluez5/dbus-monitor.c +++ b/spa/plugins/bluez5/dbus-monitor.c @@ -24,6 +24,7 @@ #include #include +#include #include @@ -48,9 +49,8 @@ struct impl struct spa_log_topic log_topic; struct spa_log *log; - DBusPendingCall *get_managed_objects_call; + struct spa_dbus_async_call get_managed_objects_call; - unsigned int filters_added:1; unsigned int objects_listed:1; struct spa_list objects[]; @@ -60,11 +60,7 @@ struct object { unsigned int removed:1; unsigned int updating:1; - union { - struct spa_dbus_object this; - /* extra data follows 'this', force safer alignment */ - long double _align; - }; + struct spa_dbus_object alignas(max_align_t) this; }; static void object_emit_update(struct object *o) @@ -292,25 +288,27 @@ static void interfaces_removed(struct impl *impl, DBusMessageIter *arg_iter) } } -static void get_managed_objects_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void get_managed_objects_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct impl *impl = SPA_CONTAINER_OF(call_ptr, struct impl, get_managed_objects_call); + struct impl *impl = SPA_CONTAINER_OF(call, struct impl, get_managed_objects_call); DBusMessageIter it[6]; if (dbus_message_is_error(r, DBUS_ERROR_UNKNOWN_METHOD)) { - spa_log_warn(impl->log, "BlueZ D-Bus ObjectManager not available"); + spa_log_warn(impl->log, "ObjectManager not available at path=%s", + impl->this.path); return; } if (dbus_message_get_type(r) == DBUS_MESSAGE_TYPE_ERROR) { - spa_log_error(impl->log, "GetManagedObjects() failed: %s", - dbus_message_get_error_name(r)); + spa_log_error(impl->log, "GetManagedObjects() at %s failed: %s", + impl->this.path, dbus_message_get_error_name(r)); return; } if (!dbus_message_iter_init(r, &it[0]) || - !spa_streq(dbus_message_get_signature(r), "a{oa{sa{sv}}}")) { - spa_log_error(impl->log, "Invalid reply signature for GetManagedObjects()"); + !dbus_message_has_signature(r, "a{oa{sa{sv}}}")) { + spa_log_error(impl->log, "Invalid reply signature for GetManagedObjects() at %s", + impl->this.path); return; } @@ -334,7 +332,7 @@ static int get_managed_objects(struct impl *impl) { DBusMessage *m; - if (impl->objects_listed || impl->get_managed_objects_call) + if (impl->objects_listed || impl->get_managed_objects_call.pending) return 0; m = dbus_message_new_method_call(impl->this.service, @@ -346,7 +344,7 @@ static int get_managed_objects(struct impl *impl) dbus_message_set_auto_start(m, false); - return spa_dbus_async_call(impl->conn, m, &impl->get_managed_objects_call, + return spa_dbus_async_call_send(&impl->get_managed_objects_call, impl->conn, m, get_managed_objects_reply); } @@ -354,9 +352,7 @@ static int get_managed_objects(struct impl *impl) static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *user_data) { struct impl *impl = user_data; - DBusError err; - - dbus_error_init(&err); + DBusError err = DBUS_ERROR_INIT; if (dbus_message_is_signal(m, DBUS_INTERFACE_DBUS, "NameOwnerChanged")) { const char *name, *old_owner, *new_owner; @@ -413,7 +409,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us if (!impl->objects_listed) goto done; - if (!dbus_message_iter_init(m, &it) || !spa_streq(dbus_message_get_signature(m), "oa{sa{sv}}")) { + if (!dbus_message_iter_init(m, &it) || !dbus_message_has_signature(m, "oa{sa{sv}}")) { spa_log_error(impl->log, "Invalid signature found in InterfacesAdded"); goto done; } @@ -427,7 +423,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us if (!impl->objects_listed) goto done; - if (!dbus_message_iter_init(m, &it) || !spa_streq(dbus_message_get_signature(m), "oas")) { + if (!dbus_message_iter_init(m, &it) || !dbus_message_has_signature(m, "oas")) { spa_log_error(impl->log, "Invalid signature found in InterfacesRemoved"); goto done; } @@ -443,7 +439,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void *us goto done; if (!dbus_message_iter_init(m, &args) || - !spa_streq(dbus_message_get_signature(m), "sa{sv}as")) { + !dbus_message_has_signature(m, "sa{sv}as")) { spa_log_error(impl->log, "Invalid signature found in PropertiesChanged"); goto done; } @@ -478,22 +474,15 @@ done: static int add_filters(struct impl *impl) { - DBusError err; + DBusError err = DBUS_ERROR_INIT; char rule[1024]; const struct spa_dbus_interface *iface; - if (impl->filters_added) - return 0; - if (!dbus_connection_add_filter(impl->conn, filter_cb, impl, NULL)) { spa_log_error(impl->log, "failed to add DBus filter"); return -EIO; } - impl->filters_added = true; - - dbus_error_init(&err); - spa_scnprintf(rule, sizeof(rule), "type='signal',sender='org.freedesktop.DBus'," "interface='org.freedesktop.DBus',member='NameOwnerChanged'," @@ -538,6 +527,8 @@ static int add_filters(struct impl *impl) return 0; fail: + dbus_connection_remove_filter(impl->conn, filter_cb, impl); + spa_log_error(impl->log, "failed to add DBus match: %s", err.message); dbus_error_free(&err); return -EIO; @@ -553,6 +544,7 @@ struct spa_dbus_monitor *spa_dbus_monitor_new(DBusConnection *conn, struct impl *impl; const struct spa_dbus_interface *iface; size_t num_interfaces = 0, i; + int res; for (iface = interfaces; iface && iface->name; ++iface) { spa_assert(iface->object_size >= sizeof(struct spa_dbus_object)); @@ -563,8 +555,6 @@ struct spa_dbus_monitor *spa_dbus_monitor_new(DBusConnection *conn, if (impl == NULL) return NULL; - dbus_connection_ref(conn); - impl->conn = conn; impl->this.service = strdup(service); impl->this.path = strdup(path); @@ -582,7 +572,16 @@ struct spa_dbus_monitor *spa_dbus_monitor_new(DBusConnection *conn, spa_assert(impl->this.service); spa_assert(impl->this.path); - add_filters(impl); + if ((res = add_filters(impl)) < 0) { + free((void *)impl->this.service); + free((void *)impl->this.path); + free(impl); + errno = -res; + return NULL; + } + + dbus_connection_ref(conn); + get_managed_objects(impl); return &impl->this; @@ -595,16 +594,9 @@ void spa_dbus_monitor_destroy(struct spa_dbus_monitor *this) struct spa_dbus_object *object; size_t i; - if (impl->filters_added) { - dbus_connection_remove_filter(impl->conn, filter_cb, impl); - impl->filters_added = false; - } + dbus_connection_remove_filter(impl->conn, filter_cb, impl); - if (impl->get_managed_objects_call) { - dbus_pending_call_cancel(impl->get_managed_objects_call); - dbus_pending_call_unref(impl->get_managed_objects_call); - impl->get_managed_objects_call = NULL; - } + spa_dbus_async_call_cancel(&impl->get_managed_objects_call); for (iface = impl->this.interfaces, i = 0; iface && iface->name; ++iface, ++i) { spa_list_consume(object, &impl->objects[i], link) { @@ -670,48 +662,38 @@ struct spa_list *spa_dbus_monitor_object_list(struct spa_dbus_monitor *this, con return object_list; } -typedef void (*pending_call_reply_t)(DBusPendingCall **, DBusMessage *); - -static dbus_int32_t async_call_reply_id = -1; - static void async_call_reply(DBusPendingCall *pending, void *user_data) { - DBusPendingCall **call_ptr = user_data; + struct spa_dbus_async_call *call = user_data; DBusMessage *r; - pending_call_reply_t reply; - spa_assert(pending == *call_ptr); - *call_ptr = NULL; + spa_assert(pending == call->pending); + call->pending = NULL; r = dbus_pending_call_steal_reply(pending); - reply = dbus_pending_call_get_data(pending, async_call_reply_id); dbus_pending_call_unref(pending); - spa_assert(reply != NULL); + spa_assert(call->reply != NULL); if (r == NULL) return; - reply(call_ptr, r); + call->reply(call, r); dbus_message_unref(r); } -int spa_dbus_async_call(DBusConnection *conn, DBusMessage *msg, - DBusPendingCall **call_ptr, - pending_call_reply_t reply) +int spa_dbus_async_call_send(struct spa_dbus_async_call *call, + DBusConnection *conn, DBusMessage *msg, + void (*reply)(struct spa_dbus_async_call *call, DBusMessage *reply)) { int res = -EIO; DBusPendingCall *pending; spa_assert(msg); - spa_assert(call_ptr); + spa_assert(conn); + spa_assert(call); - if (async_call_reply_id == -1) { - if (!dbus_pending_call_allocate_data_slot(&async_call_reply_id)) - goto done; - } - - if (*call_ptr) { + if (call->pending) { res = -EBUSY; goto done; } @@ -719,19 +701,14 @@ int spa_dbus_async_call(DBusConnection *conn, DBusMessage *msg, if (!dbus_connection_send_with_reply(conn, msg, &pending, -1)) goto done; - if (!dbus_pending_call_set_data(pending, async_call_reply_id, reply, NULL)) { + if (!dbus_pending_call_set_notify(pending, async_call_reply, call, NULL)) { dbus_pending_call_cancel(pending); dbus_pending_call_unref(pending); goto done; } - if (!dbus_pending_call_set_notify(pending, async_call_reply, call_ptr, NULL)) { - dbus_pending_call_cancel(pending); - dbus_pending_call_unref(pending); - goto done; - } - - *call_ptr = pending; + call->reply = reply; + call->pending = pending; res = 0; done: diff --git a/spa/plugins/bluez5/dbus-monitor.h b/spa/plugins/bluez5/dbus-monitor.h index 7690864cd..04281bd57 100644 --- a/spa/plugins/bluez5/dbus-monitor.h +++ b/spa/plugins/bluez5/dbus-monitor.h @@ -137,29 +137,36 @@ void spa_dbus_monitor_ignore_object(struct spa_dbus_monitor *monitor, struct spa_list *spa_dbus_monitor_object_list(struct spa_dbus_monitor *monitor, const char *interface); + +struct spa_dbus_async_call +{ + DBusPendingCall *pending; + void (*reply)(struct spa_dbus_async_call *call, DBusMessage *reply); +}; + /** * Make an asynchronous DBus call. * - * On successful return, *call_ptr contains the pending call handle. + * On successful return, call contains the pending call handle. * The same address is passed to the reply function, and can be used - * to locate the address container object. The reply function does not - * need to unref the reply, or set *call_ptr to NULL itself. + * to locate the address container object. The reply function shall not + * unref the reply. * * The msg passed in is stolen, and unref'd also on errors. * * \returns 0 on success, < 0 on error. */ -int spa_dbus_async_call(DBusConnection *conn, DBusMessage *msg, - DBusPendingCall **call_ptr, - void (*reply)(DBusPendingCall **call_ptr, DBusMessage *reply)); +int spa_dbus_async_call_send(struct spa_dbus_async_call *call, + DBusConnection *conn, DBusMessage *msg, + void (*reply)(struct spa_dbus_async_call *call, DBusMessage *reply)); /** Convenience for pending call cancel + unref */ -static inline void spa_dbus_async_call_cancel(DBusPendingCall **call_ptr) +static inline void spa_dbus_async_call_cancel(struct spa_dbus_async_call *call) { - if (*call_ptr) { - dbus_pending_call_cancel(*call_ptr); - dbus_pending_call_unref(*call_ptr); - *call_ptr = NULL; + if (call->pending) { + dbus_pending_call_cancel(call->pending); + dbus_pending_call_unref(call->pending); + spa_zero(*call); } } diff --git a/spa/plugins/bluez5/midi-enum.c b/spa/plugins/bluez5/midi-enum.c index 7396bb3ac..c0dc353f8 100644 --- a/spa/plugins/bluez5/midi-enum.c +++ b/spa/plugins/bluez5/midi-enum.c @@ -82,7 +82,7 @@ struct impl struct adapter { struct spa_dbus_object object; - DBusPendingCall *register_call; + struct spa_dbus_async_call register_call; unsigned int registered:1; }; @@ -113,8 +113,8 @@ struct chr char *service_path; char *description; uint32_t id; - DBusPendingCall *read_call; - DBusPendingCall *dsc_call; + struct spa_dbus_async_call read_call; + struct spa_dbus_async_call dsc_call; unsigned int node_emitted:1; unsigned int valid_uuid:1; unsigned int read_probed:1; @@ -175,9 +175,9 @@ static void remove_chr_node(struct impl *impl, struct chr *chr) static void check_chr_node(struct impl *impl, struct chr *chr); -static void read_probe_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void read_probe_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct chr *chr = SPA_CONTAINER_OF(call_ptr, struct chr, read_call); + struct chr *chr = SPA_CONTAINER_OF(call, struct chr, read_call); struct impl *impl = chr->object.user_data; if (dbus_message_get_type(r) == DBUS_MESSAGE_TYPE_ERROR) { @@ -206,7 +206,7 @@ static int read_probe(struct impl *impl, struct chr *chr) if (chr->read_probed) return 0; - if (chr->read_call) + if (chr->read_call.pending) return -EBUSY; chr->read_probed = true; @@ -225,7 +225,7 @@ static int read_probe(struct impl *impl, struct chr *chr) dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, "{sv}", &d); dbus_message_iter_close_container(&i, &d); - return spa_dbus_async_call(impl->conn, m, &chr->read_call, + return spa_dbus_async_call_send(&chr->read_call, impl->conn, m, read_probe_reply); } @@ -243,9 +243,9 @@ struct dsc *find_dsc(struct impl *impl, struct chr *chr) return NULL; } -static void read_dsc_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void read_dsc_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct chr *chr = SPA_CONTAINER_OF(call_ptr, struct chr, dsc_call); + struct chr *chr = SPA_CONTAINER_OF(call, struct chr, dsc_call); struct impl *impl = chr->object.user_data; DBusError err; DBusMessageIter args, arr; @@ -293,7 +293,7 @@ static int read_dsc(struct impl *impl, struct chr *chr) if (chr->dsc_probed) return 0; - if (chr->dsc_call) + if (chr->dsc_call.pending) return -EBUSY; chr->dsc_probed = true; @@ -320,7 +320,7 @@ static int read_dsc(struct impl *impl, struct chr *chr) dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, "{sv}", &d); dbus_message_iter_close_container(&i, &d); - return spa_dbus_async_call(impl->conn, m, &chr->dsc_call, + return spa_dbus_async_call_send(&chr->dsc_call, impl->conn, m, read_dsc_reply); } @@ -398,9 +398,9 @@ static void check_all_nodes(struct impl *impl) check_chr_node(impl, chr); } -static void adapter_register_application_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void adapter_register_application_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct adapter *adapter = SPA_CONTAINER_OF(call_ptr, struct adapter, register_call); + struct adapter *adapter = SPA_CONTAINER_OF(call, struct adapter, register_call); struct impl *impl = adapter->object.user_data; if (dbus_message_get_type(r) == DBUS_MESSAGE_TYPE_ERROR) { @@ -431,7 +431,7 @@ static int adapter_register_application(struct adapter *adapter) dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, "{sv}", &d); dbus_message_iter_close_container(&i, &d); - return spa_dbus_async_call(impl->conn, m, &adapter->register_call, + return spa_dbus_async_call_send(&adapter->register_call, impl->conn, m, adapter_register_application_reply); } @@ -483,7 +483,7 @@ static void adapter_update(struct spa_dbus_object *object) { struct adapter *adapter = SPA_CONTAINER_OF(object, struct adapter, object); - if (adapter->registered || adapter->register_call) + if (adapter->registered || adapter->register_call.pending) return; adapter_register_application(adapter); diff --git a/spa/plugins/bluez5/midi-node.c b/spa/plugins/bluez5/midi-node.c index 98d3cb9f7..4e9033290 100644 --- a/spa/plugins/bluez5/midi-node.c +++ b/spa/plugins/bluez5/midi-node.c @@ -151,7 +151,7 @@ struct port { struct time_sync sync; unsigned int acquired:1; - DBusPendingCall *acquire_call; + struct spa_dbus_async_call acquire_call; struct spa_source source; @@ -919,9 +919,9 @@ static int do_release(struct impl *this); static int do_stop(struct impl *this); -static void acquire_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void acquire_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct port *port = SPA_CONTAINER_OF(call_ptr, struct port, acquire_call); + struct port *port = SPA_CONTAINER_OF(call, struct port, acquire_call); struct impl *this = port->impl; const char *method = (port->direction == SPA_DIRECTION_OUTPUT) ? "AcquireNotify" : "AcquireWrite"; @@ -977,7 +977,7 @@ static int do_acquire(struct port *port) if (port->acquired) return 0; - if (port->acquire_call) + if (port->acquire_call.pending) return 0; spa_log_info(this->log, @@ -995,7 +995,7 @@ static int do_acquire(struct port *port) dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, "{sv}", &d); dbus_message_iter_close_container(&i, &d); - return spa_dbus_async_call(this->conn, m, &port->acquire_call, + return spa_dbus_async_call_send(&port->acquire_call, this->conn, m, acquire_reply); } diff --git a/spa/plugins/bluez5/midi-server.c b/spa/plugins/bluez5/midi-server.c index b30b239b7..94abcb717 100644 --- a/spa/plugins/bluez5/midi-server.c +++ b/spa/plugins/bluez5/midi-server.c @@ -68,7 +68,7 @@ struct impl struct adapter { struct spa_dbus_object object; - DBusPendingCall *register_call; + struct spa_dbus_async_call register_call; unsigned int registered:1; }; @@ -147,11 +147,10 @@ static int dsc_prop_characteristic_get(struct spa_dbus_local_object *object, DBu static int dsc_prop_flags_get(struct spa_dbus_local_object *object, DBusMessageIter *value) { DBusMessageIter a; - const char *flags[] = {"encrypt-read", NULL }; - const char **p; + static const char * const flags[] = { "encrypt-read" }; dbus_message_iter_open_container(value, DBUS_TYPE_ARRAY, "s", &a); - for (p = &flags[0]; *p; ++p) + SPA_FOR_EACH_ELEMENT_VAR(flags, p) dbus_message_iter_append_basic(&a, DBUS_TYPE_STRING, p); dbus_message_iter_close_container(value, &a); @@ -384,12 +383,11 @@ static int chr_prop_write_acquired_get(struct spa_dbus_local_object *object, DBu static int chr_prop_flags_get(struct spa_dbus_local_object *object, DBusMessageIter *value) { DBusMessageIter a; - const char *flags[] = {"encrypt-read", "write-without-response", - "encrypt-write", "encrypt-notify", NULL }; - const char **p; + static const char * const flags[] = {"encrypt-read", "write-without-response", + "encrypt-write", "encrypt-notify" }; dbus_message_iter_open_container(value, DBUS_TYPE_ARRAY, "s", &a); - for (p = &flags[0]; *p; ++p) + SPA_FOR_EACH_ELEMENT_VAR(flags, p) dbus_message_iter_append_basic(&a, DBUS_TYPE_STRING, p); dbus_message_iter_close_container(value, &a); @@ -488,9 +486,9 @@ static const struct spa_dbus_local_interface midi_service_interfaces[] = { * Adapters */ -static void adapter_register_application_reply(DBusPendingCall **call_ptr, DBusMessage *r) +static void adapter_register_application_reply(struct spa_dbus_async_call *call, DBusMessage *r) { - struct adapter *adapter = SPA_CONTAINER_OF(call_ptr, struct adapter, register_call); + struct adapter *adapter = SPA_CONTAINER_OF(call, struct adapter, register_call); struct impl *impl = adapter->object.user_data; if (dbus_message_get_type(r) == DBUS_MESSAGE_TYPE_ERROR) { @@ -521,7 +519,7 @@ static int adapter_register_application(struct adapter *adapter) dbus_message_iter_open_container(&i, DBUS_TYPE_ARRAY, "{sv}", &d); dbus_message_iter_close_container(&i, &d); - return spa_dbus_async_call(impl->conn, m, &adapter->register_call, + return spa_dbus_async_call_send(&adapter->register_call, impl->conn, m, adapter_register_application_reply); } @@ -529,7 +527,7 @@ static void adapter_update(struct spa_dbus_object *object) { struct adapter *adapter = SPA_CONTAINER_OF(object, struct adapter, object); - if (adapter->registered || adapter->register_call) + if (adapter->registered || adapter->register_call.pending) return; adapter_register_application(adapter);