Hi Frédéric,
On Thu, Jul 26, 2012, Frédéric Danis wrote:
> +static int parse_properties(DBusMessageIter *props, const char **uuid,
> + uint16_t *version, uint16_t *features)
> +{
> + gboolean has_uuid = FALSE;
> +
> + while (dbus_message_iter_get_arg_type(props) == DBUS_TYPE_DICT_ENTRY) {
> + const char *key;
> + DBusMessageIter value, entry;
> + int var;
> +
> + dbus_message_iter_recurse(props, &entry);
> + dbus_message_iter_get_basic(&entry, &key);
> +
> + dbus_message_iter_next(&entry);
> + dbus_message_iter_recurse(&entry, &value);
> +
> + var = dbus_message_iter_get_arg_type(&value);
> + if (strcasecmp(key, "UUID") == 0) {
> + if (var != DBUS_TYPE_STRING)
> + return -EINVAL;
> + dbus_message_iter_get_basic(&value, uuid);
> + has_uuid = TRUE;
> + } else if (strcasecmp(key, "Version") == 0) {
> + if (var != DBUS_TYPE_UINT16)
> + return -EINVAL;
> + dbus_message_iter_get_basic(&value, version);
> + } else if (strcasecmp(key, "Features") == 0) {
> + if (var != DBUS_TYPE_UINT16)
> + return -EINVAL;
> + dbus_message_iter_get_basic(&value, features);
> + }
> +
> + dbus_message_iter_next(props);
> + }
> +
> + return (has_uuid) ? 0 : -EINVAL;
> +}
I suppose you could just make the above function return gboolean as it
only has two possible return values.
> +static int dev_close(struct telephony_device *tel_dev)
> +{
> + int sock;
> +
> + if (tel_dev->rfcomm) {
> + sock = g_io_channel_unix_get_fd(tel_dev->rfcomm);
> + shutdown(sock, SHUT_RDWR);
> + tel_dev->rfcomm = NULL;
> + }
Looks like you're missing a g_io_channel_unref there.
> +static void hs_newconnection_reply(DBusPendingCall *call, void *user_data)
> +{
> + struct telephony_device *tel_dev = user_data;
> + DBusMessage *reply = dbus_pending_call_steal_reply(call);
> + DBusError derr;
> +
> + dbus_error_init(&derr);
> + if (!dbus_set_error_from_message(&derr, reply)) {
> + DBG("Agent reply: file descriptor passed successfully");
> + g_io_add_watch(tel_dev->rfcomm, G_IO_ERR | G_IO_HUP | G_IO_NVAL,
> + (GIOFunc) hs_dev_disconnect_cb, tel_dev);
> + headset_slc_complete(tel_dev->au_dev);
> + goto done;
> + }
Firstly, a more common way would be to test for positive return of
dbus_set_error_from_message and handle the error reply within the
if-clause. Secondly, please don't do callback typecasts (GIOFunc) but
instead just assign to the right type inside the callback function
itself.
> +static void get_record_cb(sdp_list_t *recs, int err, gpointer user_data)
> +{
> + struct telephony_device *tel_dev = user_data;
Here you do the right kind of handling of callback types. Why the
inconsistency?
> + sdp_get_profile_descs(recs->data, &profiles);
> + if (profiles == NULL)
> + goto failed;
I think it'd be cleaner/simpler to do:
if (sdp_get_profile_descs(...) < 0)
goto failed;
> + desc = profiles->data;
> +
> + if (sdp_uuid16_cmp(&desc->uuid, &uuid) == 0)
> + tel_dev->version = desc->version;
I don't think it's safe to assume that what's returned by
sdp_get_profile_descs is always a uuid16. Instead using sdp_uuid_cmp()
would seem more appropriate.
> +struct telephony_device *telephony_device_connecting(GIOChannel *io,
> + struct btd_device *btd_dev,
> + struct audio_device *au_dev,
> + const char *uuid)
> +{
> + struct btd_adapter *adapter;
> + struct telephony_agent *agent;
> + struct telephony_device *tel_dev;
> + uuid_t r_uuid;
> + int err;
> +
> + adapter = device_get_adapter(btd_dev);
> + agent = find_agent(adapter, NULL, NULL, uuid);
> + if (agent == NULL)
> + return NULL;
> +
> + tel_dev = g_new0(struct telephony_device, 1);
> + tel_dev->btd_dev = btd_device_ref(btd_dev);
> + tel_dev->name = g_strdup(agent->name);
> + tel_dev->path = g_strdup(agent->path);
> + tel_dev->config = agent->config;
> + tel_dev->au_dev = au_dev;
> + tel_dev->rfcomm = io;
Missing g_io_channel_ref here.
> + err = bt_search_service(&au_dev->src, &au_dev->dst, &r_uuid,
> + get_record_cb, tel_dev, NULL);
> + if (err < 0) {
> + telephony_device_disconnect(tel_dev);
> + return NULL;
> + }
> + tel_dev->pending_sdp = TRUE;
An empty line should follow after }
> +void telephony_device_disconnect(struct telephony_device *device)
> +{
> + dev_close(device);
> +
> + if (device->pending_sdp)
> + return;
Shouldn't you cancel the SDP operation here with bt_cancel_discovery?
> +gboolean telephony_get_ready_state(struct btd_adapter *adapter)
> +{
> + return find_agent(adapter, NULL, NULL, HFP_AG_UUID) ? TRUE : FALSE;
> +}
If such a function is needed just call it telephony_is_ready. It makes
the calling side look more natural: "if (telephony_is_ready(adapter))".
> +static int register_interface(struct btd_adapter *adapter)
> +{
> + const char *path;
> +
> + path = adapter_get_path(adapter);
> +
> + if (!g_dbus_register_interface(connection, path,
> + AUDIO_TELEPHONY_INTERFACE,
> + telsrv_methods, NULL,
> + NULL, adapter, path_unregister)) {
> + error("D-Bus failed to register %s interface",
> + AUDIO_TELEPHONY_INTERFACE);
> + return -1;
> + }
> +
> + DBG("Registered interface %s", AUDIO_TELEPHONY_INTERFACE);
> +
> + return 0;
> +}
> +
> +static void unregister_interface(struct btd_adapter *adapter)
> +{
> + g_dbus_unregister_interface(connection, adapter_get_path(adapter),
> + AUDIO_TELEPHONY_INTERFACE);
> +}
> +
> +int telephony_adapter_init(struct btd_adapter *adapter)
> +{
> + DBG("adapter: %p", adapter);
> +
> + return register_interface(adapter);
> +}
> +
> +void telephony_adapter_exit(struct btd_adapter *adapter)
> +{
> + struct telephony_agent *agent;
> +
> + DBG("adapter: %p", adapter);
> +
> + unregister_interface(adapter);
> +
> + while ((agent = find_agent(adapter, NULL, NULL, NULL)) != NULL) {
> + agents = g_slist_remove(agents, agent);
> + free_agent(agent);
> + }
> +}
The register_interface and unregister_interface functions above seem
unnecessary to me. Just include their code directly within
telephony_adapter_init and telephony_adapter_exit.
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
