On Thu, 2014-06-05 at 10:45 -0700, eu at felipetonello.com wrote:
> From: "Felipe F. Tonello" <eu at felipetonello.com>
>
> This module is used to change a card profile when a voice call is received
> from oFono.
>
> Signed-off-by: Felipe F. Tonello <eu at felipetonello.com>
> ---
> src/Makefile.am | 12 +-
> src/modules/module-ofono-switch-on-voicecall.c | 480 +++++++++++++++++++++++++
> 2 files changed, 490 insertions(+), 2 deletions(-)
> create mode 100644 src/modules/module-ofono-switch-on-voicecall.c
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 1ac8a16..add00fb 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1326,7 +1326,8 @@ endif
> if HAVE_DBUS
> modlibexec_LTLIBRARIES += \
> module-rygel-media-server.la \
> - module-dbus-protocol.la
> + module-dbus-protocol.la \
> + module-ofono-switch-on-voicecall.la
> endif
>
> if HAVE_BLUEZ
> @@ -1457,7 +1458,8 @@ SYMDEF_FILES = \
> module-switch-on-connect-symdef.h \
> module-switch-on-port-available-symdef.h \
> module-filter-apply-symdef.h \
> - module-filter-heuristics-symdef.h
> + module-filter-heuristics-symdef.h \
> + module-ofono-switch-on-voicecall-symdef.h
>
> if HAVE_ESOUND
> SYMDEF_FILES += \
> @@ -2103,6 +2105,12 @@ module_rygel_media_server_la_LDFLAGS = $(MODULE_LDFLAGS)
> module_rygel_media_server_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) libprotocol-http.la
> module_rygel_media_server_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
>
> +# oFono VoiceCall switcher
> +module_ofono_switch_on_voicecall_la_SOURCES = modules/module-ofono-switch-on-voicecall.c
> +module_ofono_switch_on_voicecall_la_LDFLAGS = $(MODULE_LDFLAGS)
> +module_ofono_switch_on_voicecall_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS)
> +module_ofono_switch_on_voicecall_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
> +
> ###################################
> # Some minor stuff #
> ###################################
> diff --git a/src/modules/module-ofono-switch-on-voicecall.c b/src/modules/module-ofono-switch-on-voicecall.c
> new file mode 100644
> index 0000000..786aedb
> --- /dev/null
> +++ b/src/modules/module-ofono-switch-on-voicecall.c
> @@ -0,0 +1,480 @@
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2013 Felipe F. Tonello <eu at felipetonello.com>
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + PulseAudio is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with PulseAudio; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> + USA.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <pulsecore/module.h>
> +#include <pulsecore/modargs.h>
> +#include <pulsecore/namereg.h>
> +#include <pulsecore/core-util.h>
> +#include <pulsecore/dbus-shared.h>
> +#include <pulsecore/strlist.h>
> +#include <pulsecore/hashmap.h>
> +
> +#include "module-ofono-switch-on-voicecall-symdef.h"
> +
> +PA_MODULE_AUTHOR("Felipe F. Tonello");
> +PA_MODULE_DESCRIPTION("Card profile switcher while a call detected by oFono");
> +PA_MODULE_VERSION(PACKAGE_VERSION);
> +PA_MODULE_LOAD_ONCE(false);
> +PA_MODULE_USAGE("card=<card to switch profile> profile=<profile to set during a call>");
> +
> +static const char* const valid_modargs[] = {
> + "card",
Don't use tabs for indentation.
http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/CodingStyle/
> + "profile",
> + NULL,
> +};
> +
> +struct userdata {
> + pa_card *card;
This pointer becomes stale when the card is removed. Use
PA_CORE_HOOK_CARD_UNLINK to get notified when the card is getting
removed.
> + pa_card_profile *profile;
> + pa_card_profile *old_profile;
> + pa_dbus_connection *bus;
> + bool filter_added;
> + pa_hashmap *modems_hash;
> + char *voicecall_path;
> +};
> +
> +static void modems_hash_free_value(void *p) {
> + pa_strlist_free((pa_strlist *)p);
pa_strlist_free() only frees the list structure, the contained strings
are leaked.
> +}
> +
> +static const char *check_variant_property(DBusMessageIter *i) {
> + const char *key;
> +
> + pa_assert(i);
> +
> + if (dbus_message_iter_get_arg_type(i) != DBUS_TYPE_STRING) {
> + pa_log("Property name not a string.");
> + return NULL;
> + }
> +
> + dbus_message_iter_get_basic(i, &key);
> +
> + if (!dbus_message_iter_next(i)) {
> + pa_log("Property value missing");
> + return NULL;
> + }
> +
> + if (dbus_message_iter_get_arg_type(i) != DBUS_TYPE_VARIANT) {
> + pa_log("Property value not a variant.");
> + return NULL;
> + }
> +
> + return key;
> +}
This function would be shorter if you'd check the iterator signature.
That way there would be only one place that can fail, instead of three.
> +
> +static inline bool update_profile(const struct userdata *u, pa_card_profile *p) {
> + pa_assert(u);
> + pa_assert(p);
> +
> + /* only change profile if the new profile is different */
> + if (u->card->active_profile == p)
> + return false;
> +
> + if (pa_card_set_profile(u->card, p, false) < 0) {
> + pa_log_error("Couldn't set profile '%s' for card '%s'", p->name, u->card->name);
> + return false;
> + }
> +
> + pa_log_info("Set profile '%s' for card '%s'", p->name, u->card->name);
> +
> + return true;
> +}
> +
> +static bool switch_profile(const struct userdata *u) {
> + return update_profile(u, u->profile);
> +}
> +
> +static bool switch_back_profile(const struct userdata *u) {
> + return update_profile(u, u->old_profile);
> +}
> +
> +static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *message, void *userdata) {
> + struct userdata *u = userdata;
> + DBusError error;
> +
> + pa_assert(bus);
> + pa_assert(message);
> + pa_assert(u);
> +
> + dbus_error_init(&error);
> +
> + pa_log_debug("dbus: interface=%s, path=%s, member=%s\n",
> + dbus_message_get_interface(message),
> + dbus_message_get_path(message),
> + dbus_message_get_member(message));
This will log every D-Bus message we receive, which I think is too
spammy.
> +
> + if (dbus_message_is_signal(message, "org.ofono.Manager", "ModemAdded")) {
> + /* create new listener for the modem */
> +
> + const char *path;
> + pa_strlist *filter_list = NULL;
> + char *call_added, *call_removed;
> +
> + if (!dbus_message_get_args(message, &error, DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_INVALID)) {
> + pa_log_error("Failed to parse ModemAdded message: %s: %s", error.name, error.message);
> + goto finish;
> + }
> +
> + pa_log_debug("ModemAdded(object path, dict properties): path=%s", path);
> +
> + if (pa_hashmap_get(u->modems_hash, path)) {
> + pa_log_info("Modem already loaded: %s", path);
> + goto finish;
> + }
> +
> + call_added = pa_sprintf_malloc("type='signal',sender='org.ofono',"
> + "interface='org.ofono.VoiceCallManager',"
> + "path='%s',member='CallAdded'", path);
> + call_removed = pa_sprintf_malloc("type='signal',sender='org.ofono',"
> + "interface='org.ofono.VoiceCallManager',"
> + "path='%s',member='CallRemoved'", path);
> +
> + if (pa_dbus_add_matches(pa_dbus_connection_get(u->bus), &error,
> + call_added, call_removed, NULL) < 0) {
> + pa_log_error("Unable to subscribe to oFono VoiceCallManager signals: %s: %s",
> + error.name, error.message);
> + pa_xfree(call_added);
> + pa_xfree(call_removed);
> + goto finish;
> + }
> +
> + filter_list = pa_strlist_prepend(filter_list, call_added);
> + filter_list = pa_strlist_prepend(filter_list, call_removed);
> + pa_hashmap_put(u->modems_hash, (char *)path, filter_list);
You can't use the path variable as the key, the pointer becomes stale as
soon as the message is freed.
Isn't the result of this that we will listen for every CallAdded and
CallRemoved signal? If so, we don't need to register separate matches
for each modem, we can register just one match for all CallAdded and
CallRemoved signals.
It looks like you don't query the current list of modems nor the current
list of calls when the module is loaded. This means that if there
already is a modem or a call when the module is loaded, the module
starts in wrong state.
There's this hierarchy of objects to track:
Manager
??Modem1
? ??VoiceCall1
? ??VoiceCall2
??Modem2
??VoiceCall3
??VoiceCall4
Something to note is that only modems that have
"org.ofono.VoiceCallManager" in their "Interfaces" property should be
queried for VoiceCall objects.
So, I propose that the module works like this:
1. Register matches for ModemAdded, ModemRemoved, CallAdded, CallRemoved
and PropertyChanged (for org.ofono.VoiceCall interface only). Register
them in pa__init() instead of registering per-object matches.
2. Call Manager.GetModems().
3. When you get a reply to Manager.GetModems(), call
VoiceCallManager.GetCalls() on those modems that support the
"org.ofono.VoiceCallManager" interface.
4. When you get a reply to VoiceCallManager.GetCalls(), you have the
full state that you need. State updates will be provided via the
signals.
5. In the ModemAdded handler, call VoiceCallManager.GetCalls() to get
the current calls of the new modem.
> +
> + } else if (dbus_message_is_signal(message, "org.ofono.Manager", "ModemRemoved")) {
> + /* remove that listener */
> + const char *path;
> + pa_strlist *filter_list = NULL, *l;
> + char *s;
> +
> + if (!dbus_message_get_args(message, &error, DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_INVALID)) {
> + pa_log_error("Failed to parse ModemAdded message: %s: %s", error.name, error.message);
> + goto finish;
> + }
> +
> + pa_log("ModemRemoved(object path): path=%s", path);
> +
> + if (!(filter_list = pa_hashmap_remove(u->modems_hash, path))) {
> + pa_log_info("Modem '%s' not loaded", path);
> + goto finish;
> + }
> +
> + l = filter_list;
> + while (l) {
> + l = pa_strlist_pop(l, &s);
> + pa_dbus_remove_matches(pa_dbus_connection_get(u->bus), s, NULL);
> + pa_xfree(s);
> + }
> +
> + } else if (dbus_message_is_signal(message, "org.ofono.VoiceCallManager", "CallAdded")) {
> + /* check call properties if "state" is "dialing" or "incoming" */
> + DBusMessageIter arg_i;
> +
> + if (u->voicecall_path) {
> + pa_log_error("VoiceCall was already loaded: '%s'", u->voicecall_path);
It's not an error if there are multiple simultaneous calls, so don't log
at error level.
> + goto finish;
> + }
> +
> + if (!dbus_message_iter_init(message, &arg_i)) {
> + pa_log_error("Failed to parse ModemAdded: %s: %s", error.name, error.message);
error is unset here.
> + goto finish;
> + }
> +
> + while (dbus_message_iter_get_arg_type(&arg_i) != DBUS_TYPE_INVALID) {
The arguments are expected to be one object path and then one array, not
a random collection of object paths and arrays. Don't use a loop here.
> + const char *path;
> +
> + switch (dbus_message_iter_get_arg_type(&arg_i)) {
> + case DBUS_TYPE_OBJECT_PATH: {
> + dbus_message_iter_get_basic(&arg_i, &path);
> + break;
> + }
> +
> + case DBUS_TYPE_ARRAY: {
> + DBusMessageIter array_i;
> + dbus_message_iter_recurse(&arg_i, &array_i);
> + while (dbus_message_iter_get_arg_type(&array_i) != DBUS_TYPE_INVALID) {
> + DBusMessageIter dict_i;
> +
> + dbus_message_iter_recurse(&array_i, &dict_i);
This recursion is done without checking the arg type in array_i. Use
dbus_message_get_signature() to check that the message signature is what
you expect.
> + while (dbus_message_iter_get_arg_type(&dict_i) != DBUS_TYPE_INVALID) {
dict_i is expected to contain one string and then one variant. Don't use
a loop here.
> + const char *key;
> +
> + key = check_variant_property(&dict_i);
> + if (key) {
One level of indentation can be avoided if you do
if (!key)
goto finish;
> + DBusMessageIter variant_i;
> +
> + dbus_message_iter_recurse(&dict_i, &variant_i);
> +
> + if (dbus_message_iter_get_arg_type(&variant_i) == DBUS_TYPE_STRING &&
> + pa_streq(key, "State")) {
I'd like to get an error message if there's a "State" property whose
type is not string, so I suggest:
if (!pa_streq(key, "State"))
continue;
if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_STRING) {
pa_log("Unexpected type for the State property of VoiceCall.");
goto finish;
}
That will also get rid of one level of indentation.
> + const char *state;
> + dbus_message_iter_get_basic(&variant_i, &state);
> +
> + if (pa_streq(state, "dialing") || pa_streq(state, "incoming")) {
> + char *filter;
> +
> + pa_log_debug("CallAdded(object path, dict properties): path=%s "
> + "state=%s", path, state);
> +
> + u->voicecall_path = pa_xstrdup(path);
This will result in a memory leak if the State property appears multiple
times in the dictionary.
> + filter = pa_sprintf_malloc("type='signal',sender='org.ofono',"
> + "interface='org.ofono.VoiceCall',"
> + "path='%s',member='PropertyChanged'",
> + u->voicecall_path);
> +
> + if (pa_dbus_add_matches(pa_dbus_connection_get(u->bus), &error,
> + filter, NULL) < 0) {
> + pa_log_error("Unable to subscribe to oFono VoiceCallManager signals: %s: %s",
> + error.name, error.message);
> + pa_xfree(filter);
> + goto finish;
> + }
> + pa_xfree(filter);
> + }
> + }
> + }
> + dbus_message_iter_next(&dict_i);
> + }
> +
> + dbus_message_iter_next(&array_i);
> + }
> + break;
> + }
> + }
> + dbus_message_iter_next(&arg_i);
> + }
> + } else if (dbus_message_is_signal(message, "org.ofono.VoiceCallManager", "CallRemoved")) {
> + const char *path;
> + char *filter;
> +
> + if (!dbus_message_get_args(message, &error, DBUS_TYPE_OBJECT_PATH, &path, DBUS_TYPE_INVALID)) {
> + pa_log_error("Failed to parse ModemAdded message: %s: %s", error.name, error.message);
> + goto finish;
> + }
> +
> + pa_log_debug("CallRemoved(object path, dict properties): path=%s", path);
> +
> + if (!(u->voicecall_path && pa_streq(path, u->voicecall_path))) {
> + pa_log_error("VoiceCall '%s' not loaded", path);
It's not an error to have multiple VoiceCalls.
> + goto finish;
> + }
> +
> + switch_back_profile(u);
> +
> + filter = pa_sprintf_malloc("type='signal',sender='org.ofono',"
> + "interface='org.ofono.VoiceCall',"
> + "path='%s',member='PropertyChanged'",
> + u->voicecall_path);
> +
> + pa_dbus_remove_matches(pa_dbus_connection_get(u->bus), filter, NULL);
> +
> + pa_xfree(filter);
> + pa_xfree(u->voicecall_path);
> + u->voicecall_path = NULL;
> +
> + } else if (dbus_message_is_signal(message, "org.ofono.VoiceCall", "PropertyChanged")) {
> + const char *path, *key;
> + DBusMessageIter arg_i, variant_i;
> +
> + path = dbus_message_get_path(message);
> +
> + if (!(u->voicecall_path && pa_streq(path, u->voicecall_path))) {
> + pa_log_error("VoiceCall '%s' not loaded", path);
It's not an error to have multiple VoiceCalls.
> + goto finish;
> + }
> +
> + if (!dbus_message_iter_init(message, &arg_i)) {
> + pa_log_error("Failed to parse PropertyChange: %s: %s", error.name, error.message);
error is unset here.
> + goto finish;
> + }
> +
> + key = check_variant_property(&arg_i);
> + if (!key)
> + goto finish;
> +
> + dbus_message_iter_recurse(&arg_i, &variant_i);
> +
> + if (dbus_message_iter_get_arg_type(&variant_i) == DBUS_TYPE_STRING &&
> + pa_streq(key, "State")) {
The same suggestion as earlier for checking the property type.
> + const char *state;
> +
> + dbus_message_iter_get_basic(&variant_i, &state);
> + if (pa_streq(state, "alerting") || pa_streq(state, "active")) {
> +
> + pa_log_debug("CallAdded(object path, dict properties): path=%s "
> + "state=%s", path, state);
> +
> + switch_profile(u);
> + }
Hmm, it seems that the "dialing" and "incoming" states that were checked
in the CallAdded handler are only used for setting up the match rule for
VoiceCall.PropertyChanged. I think you should ignore the "dialing" and
"incoming" states, and register for all VoiceCall.PropertyChanged
signals already in pa__init(), and track the state of all VoiceCalls. If
any of the VoiceCalls is "alerting" or "active", switch the profile. The
CallAdded handler should check for "alerting" and "active" too, unless
you know for sure that they can never appear in CallAdded (the ofono
documentation doesn't say).
> + }
> + }
> +
> +finish:
> + dbus_error_free(&error);
> +
> + return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> +}
> +
> +int pa__init(pa_module *m) {
> + pa_modargs *ma = NULL;
> + struct userdata *u = NULL;
> + char *card_name = NULL;
> + char *profile_name = NULL;
> + pa_card *card = NULL;
> + pa_card_profile *profile = NULL;
> + void *state;
> + DBusError error;
> +
> + pa_assert(m);
> +
> + dbus_error_init(&error);
> +
> + if (!(ma = pa_modargs_new(m->argument, valid_modargs))) {
> + pa_log_error("Failed to parse module arguments");
> + goto fail;
> + }
> +
> + m->userdata = u = pa_xnew0(struct userdata, 1);
> + u->modems_hash = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func,
> + NULL, modems_hash_free_value);
> +
> + if (!(card_name = pa_xstrdup(pa_modargs_get_value(ma, "card", NULL)))) {
There appears to be no reason to copy the string.
> + pa_log_error("Missing card name");
> + goto fail;
> + }
> +
> + if (!(profile_name = pa_xstrdup(pa_modargs_get_value(ma, "profile", NULL)))) {
There appears to be no reason to copy the string.
> + pa_log_error("Missing profile name");
> + goto fail;
> + }
> +
> + if (!(card = pa_namereg_get(m->core, card_name, PA_NAMEREG_CARD))) {
> + pa_log_error("No such card (%s)", card_name);
> + goto fail;
> + }
We should avoid introducing load order dependencies between modules. If
the card is not present now, it may appear later. Use
PA_CORE_HOOK_CARD_PUT to get notified about new cards.
> +
> + u->card = card;
> +
> + PA_HASHMAP_FOREACH(profile, card->profiles, state)
> + if (profile && pa_streq(profile->name, profile_name)) {
profile is always non-NULL, no need to check it.
> + u->profile = profile;
> + break;
> + }
> +
> + if (!profile) {
I think checking for u->profile would be clearer (no need to think about
what state PA_HASHMAP_FOREACH leaves the variable after the last
iteration).
> + pa_log_error("No such profile (%s) associated with card (%s)", profile_name, card_name);
> + goto fail;
> + }
> +
> + u->old_profile = card->active_profile;
> +
> + if (!(u->bus = pa_dbus_bus_get(m->core, DBUS_BUS_SYSTEM, &error))) {
> + pa_log_error("Failed to get system bus connection: %s: %s", error.name, error.message);
> + goto fail;
> + }
> +
> + if (!dbus_connection_add_filter(pa_dbus_connection_get(u->bus), filter_cb, u, NULL)) {
> + pa_log_error("Failed to add D-Bus filter function");
> + goto fail;
> + }
> +
> + u->filter_added = true;
> +
> + if (pa_dbus_add_matches(pa_dbus_connection_get(u->bus), &error,
> + "type='signal',sender='org.ofono',"
> + "interface='org.ofono.Manager',"
> + "path='/'",
> + NULL) < 0) {
> + pa_log_error("Unable to subscribe to oFono signals: %s: %s", error.name, error.message);
> + goto fail;
> + }
> +
> + pa_modargs_free(ma);
> + pa_xfree(card_name);
> + pa_xfree(profile_name);
> +
> + return 0;
> +
> +fail:
> + if (ma)
> + pa_modargs_free(ma);
> +
> + pa_xfree(card_name);
> + pa_xfree(profile_name);
> +
> + /* Make sure we don't change anything in pa__done() */
> + if (u) {
> + u->card = NULL;
> + u->old_profile = NULL;
> + }
This seems to be unnecessary. old_profile can only be either NULL or the
original card profile. pa__init() doesn't change the card profile, so
it's a no-op in pa__done() if it sets the card profile to old_profile,
but even if pa__init() did change the profile, it would only be a good
thing to restore the card profile to old_profile in case pa__init()
fails.
> +
> + dbus_error_free(&error);
> +
> + pa__done(m);
> + return -1;
> +}
> +
> +void pa__done(pa_module *m) {
> + struct userdata *u;
> +
> + pa_assert(m);
> +
> + if (!(u = m->userdata))
> + return;
> +
> + if (u->modems_hash)
> + pa_hashmap_free(u->modems_hash);
> +
> + if (u->card && u->old_profile)
> + pa_card_set_profile(u->card, u->old_profile, true);
The "save" flag should be false.
> +
> + if (u->bus) {
> +
> + if (u->voicecall_path) {
> + char *filter = pa_sprintf_malloc("type='signal',sender='org.ofono',"
> + "interface='org.ofono.VoiceCall',"
> + "path='%s',member='PropertyChanged'",
> + u->voicecall_path);
> +
> + pa_dbus_remove_matches(pa_dbus_connection_get(u->bus), filter, NULL);
> + pa_xfree(filter);
> + pa_xfree(u->voicecall_path);
> + }
> +
> + pa_dbus_remove_matches(pa_dbus_connection_get(u->bus),
> + "type='signal',sender='org.ofono',"
> + "interface='org.ofono.Manager',"
> + "path='/'",
> + NULL);
There should be a "matches_added" flag that you check here before
removing the matches. If you haven't added those matches yourself, this
may remove some other module's match rules.
--
Tanu
