On Thu, 2013-01-17 at 20:55 +0100, poljar (Damir Jeli?) wrote:
> Notification events can be error prone, this patch removes the use of
> notification events from card-restore and replaces them with hooks.
> ---
> src/modules/module-card-restore.c | 241 ++++++++++++++++++++++++++++----------
> 1 file changed, 178 insertions(+), 63 deletions(-)
>
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index 643e074..d515f37 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -62,10 +62,14 @@ static const char* const valid_modargs[] = {
> struct userdata {
> pa_core *core;
> pa_module *module;
> - pa_subscription *subscription;
> pa_hook_slot *card_new_hook_slot;
> + pa_hook_slot *card_put_hook_slot;
> + pa_hook_slot *card_profile_hook_slot;
> + pa_hook_slot *port_new_hook_slot;
> + pa_hook_slot *port_offset_hook_slot;
> pa_time_event *save_time_event;
> pa_database *database;
> + bool hooks_connected;
> };
>
> #define ENTRY_VERSION 2
> @@ -110,6 +114,27 @@ static struct entry* entry_new(void) {
> return r;
> }
>
> +static struct port_info *port_info_new(pa_device_port *port) {
> + struct port_info *p_info;
> +
> + if (port) {
> + p_info = pa_xnew(struct port_info, 1);
> + p_info->name = pa_xstrdup(port->name);
> + p_info->offset = port->latency_offset;
> + } else
> + p_info = pa_xnew0(struct port_info, 1);
> +
> + return p_info;
> +}
> +
> +static void port_info_clone(struct port_info *p_info, pa_device_port *port) {
"port_info_clone" sounds like the function copies a port_info struct
into another port_info struct. What would you think about naming this
function "port_info_update"? I was going to suggest "port_info_init",
but the context where this is used is more like updating the struct.
> + pa_assert(p_info);
> + pa_assert(port);
> +
> + p_info->name = pa_xstrdup(port->name);
The old name needs to be freed.
> + p_info->offset = port->latency_offset;
> +}
> +
> static void port_info_free(struct port_info *p_info, void *userdata) {
> pa_assert(p_info);
>
> @@ -126,6 +151,44 @@ static void entry_free(struct entry* e) {
> pa_xfree(e);
> }
>
> +static void entry_prepare_full(struct entry *entry , pa_card *card) {
Extra space before the comma.
> + struct port_info *p_info;
> + pa_device_port *port;
> + void *state;
> +
> + if (card->save_profile)
> + entry->profile = pa_xstrdup(card->active_profile->name);
> +
> + PA_HASHMAP_FOREACH(port, card->ports, state) {
> + p_info = port_info_new(port);
> + pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> + }
> +}
> +
> +static bool entrys_equal(struct entry *a, struct entry *b) {
> + void *state;
> +
> + if (!pa_streq(a->profile, b->profile) ||
> + pa_hashmap_size(a->ports) != pa_hashmap_size(b->ports))
> + return false;
> +
> + else {
I'd prefer leaving the else out to reduce the indentation. It's not
needed, since the if branch returns.
> + struct port_info *Ap_info, *Bp_info;
> +
> + PA_HASHMAP_FOREACH(Ap_info, a->ports, state) {
> + if ((Bp_info = pa_hashmap_get(b->ports, Ap_info->name))) {
> + if (Ap_info->offset != Bp_info->offset) {
> + return false;
> + }
Redundant braces in the inner if. (Technically also in the outer if, but
I prefer keeping the braces whenever there's more than one line in the
if block content. Other maintainers have different opinions about this,
so deal with the outer if as you want).
> +
> + } else {
> + return false;
> + }
Redundant braces.
> + }
> + }
> + return true;
> +}
> +
> static pa_bool_t entry_write(struct userdata *u, const char *name, const struct entry *e) {
> pa_tagstruct *t;
> pa_datum key, data;
> @@ -245,7 +308,7 @@ static struct entry* entry_read(struct userdata *u, const char *name) {
> pa_tagstruct_gets64(t, &port_offset) < 0)
> goto fail;
>
> - p_info = pa_xnew(struct port_info, 1);
> + p_info = port_info_new(NULL);
> p_info->name = pa_xstrdup(port_name);
> p_info->offset = port_offset;
>
> @@ -286,82 +349,134 @@ fail:
> return NULL;
> }
>
> -static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint32_t idx, void *userdata) {
> - struct userdata *u = userdata;
> +static void show_full_info (pa_card *card) {
Extra space before the opening brace.
> + pa_assert(card);
> +
> + if (card->save_profile)
> + pa_log_info("Storing profile and port latency offsets for card %s.", card->name);
> + else
> + pa_log_info("Storing port latency offsets for card %s.", card->name);
> +}
> +
> +static pa_hook_result_t card_put_hook_callback(pa_core *c, pa_card *card, struct userdata *u) {
> struct entry *entry, *old;
> - void *state;
> - pa_card *card;
> - pa_device_port *p;
> - struct port_info *p_info;
>
> pa_assert(c);
> - pa_assert(u);
>
> - if (t != (PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW) &&
> - t != (PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_CHANGE))
> - return;
> + entry = entry_new();
> + entry_prepare_full(entry, card);
>
> - if (!(card = pa_idxset_get_by_index(c->cards, idx)))
> - return;
> + if ((old = entry_read(u, card->name))) {
> + if (entrys_equal(entry, old))
> + goto finish;
> + else if (!card->save_profile)
> + entry->profile = pa_xstrdup(old->profile);
> + }
>
> - entry = entry_new();
> + show_full_info(card);
>
> - if (card->save_profile)
> - entry->profile = pa_xstrdup(card->active_profile->name);
> + if (entry_write(u, card->name, entry))
> + trigger_save(u);
>
> - PA_HASHMAP_FOREACH(p, card->ports, state) {
> - p_info = pa_xnew(struct port_info, 1);
> - p_info->name = pa_xstrdup(p->name);
> - p_info->offset = p->latency_offset;
> +finish:
> + entry_free(entry);
> + if (old)
> + entry_free(old);
>
> - pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> + return PA_HOOK_OK;
> +}
> +
> +static pa_hook_result_t card_profile_change_callback(pa_core *c, pa_card *card, struct userdata *u) {
> + struct entry *entry;
> +
> + pa_assert(card);
> +
> + if (!card->save_profile)
> + return PA_HOOK_OK;
> +
> + entry = entry_new();
> + if ((entry = entry_read(u, card->name))) {
Memory leak: you create a new entry, and then you immediately create
another and lose the reference to the first one.
Instead of creating the entry with entry_new() here, I suggest that you
create function entry_from_card(), which is otherwise the same as
entry_prepare_full(), but it also allocates a new entry object. Then,
replace the entry_prepare_full() calls with entry_from_card() calls.
> + entry->profile = pa_xstrdup(card->active_profile->name);
> + pa_log_info("Storing card profile for card %s.", card->name);
> + } else {
> + entry_prepare_full(entry, card);
> + show_full_info(card);
> }
>
> - if ((old = entry_read(u, card->name))) {
> - bool dirty = false;
> + if (entry_write(u, card->name, entry))
> + trigger_save(u);
>
> - if (!card->save_profile)
> - entry->profile = pa_xstrdup(old->profile);
> + entry_free(entry);
> + return PA_HOOK_OK;
> +}
>
> - if (!pa_streq(old->profile, entry->profile) ||
> - pa_hashmap_size(old->ports) != pa_hashmap_size(entry->ports))
> - dirty = true;
> +static pa_hook_result_t card_port_add_callback(pa_core *c, pa_device_port *port, struct userdata *u) {
> + struct entry *entry;
> + pa_card *card;
>
> - else {
> - struct port_info *old_p_info;
> + pa_assert(port);
> + card = port->card;
>
> - PA_HASHMAP_FOREACH(old_p_info, old->ports, state) {
> - if ((p_info = pa_hashmap_get(entry->ports, old_p_info->name))) {
> - if (p_info->offset != old_p_info->offset) {
> - dirty = true;
> - break;
> - }
> + entry = entry_new();
> + if ((entry = entry_read(u, card->name))) {
Same leak as earlier.
> + struct port_info *p_info;
>
> - } else {
> - dirty = true;
> - break;
> - }
> - }
> + if ((p_info = pa_hashmap_get(entry->ports, port->name))) {
> + port_info_clone(p_info, port);
> + } else {
Redundant braces.
> + p_info = port_info_new(port);
> + pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> }
>
> - if (!dirty) {
> - entry_free(old);
> - entry_free(entry);
> - return;
> - }
> + pa_log_info("Storing port info for port %s on card %s.",port->name, card->name);
Missing space before "port->name".
>
> - entry_free(old);
> + } else {
> + entry_prepare_full(entry, card);
> + show_full_info(card);
> }
>
> - if (card->save_profile)
> - pa_log_info("Storing profile and port latency offsets for card %s.", card->name);
> - else
> - pa_log_info("Storing port latency offsets for card %s.", card->name);
> + if (entry_write(u, card->name, entry))
> + trigger_save(u);
> +
> + entry_free(entry);
> + return PA_HOOK_OK;
> +}
> +
> +static pa_hook_result_t port_offset_change_callback(pa_core *c, pa_device_port *port, struct userdata *u) {
> + struct entry *entry;
> + pa_card *card;
> +
> + pa_assert(port);
> + card = port->card;
> +
> + entry = entry_new();
> +
> + if ((entry = entry_read(u, card->name))) {
> + struct port_info *p_info;
> +
> + if ((p_info = pa_hashmap_get(entry->ports, port->name))) {
> + p_info->offset = port->latency_offset;
> + } else {
Redundant braces.
> + p_info = pa_xnew(struct port_info, 1);
> +
> + p_info->name = pa_xstrdup(port->name);
> + p_info->offset = port->latency_offset;
Forgot to use port_info_new().
> +
> + pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
> + }
> +
> + pa_log_info("Storing latency offset for port %s on card %s.",port->name, card->name);
Missing space before "port->name".
> +
> + } else {
> + entry_prepare_full(entry, card);
> + show_full_info(card);
> + }
>
> if (entry_write(u, card->name, entry))
> trigger_save(u);
>
> entry_free(entry);
> + return PA_HOOK_OK;
> }
>
> static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
> @@ -403,8 +518,6 @@ int pa__init(pa_module*m) {
> pa_modargs *ma = NULL;
> struct userdata *u;
> char *fname;
> - pa_card *card;
> - uint32_t idx;
>
> pa_assert(m);
>
> @@ -417,9 +530,12 @@ int pa__init(pa_module*m) {
> u->core = m->core;
> u->module = m;
>
> - u->subscription = pa_subscription_new(m->core, PA_SUBSCRIPTION_MASK_CARD, subscribe_callback, u);
> -
> u->card_new_hook_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_NEW], PA_HOOK_EARLY, (pa_hook_cb_t) card_new_hook_callback, u);
> + u->card_put_hook_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_PUT], PA_HOOK_NORMAL, (pa_hook_cb_t) card_put_hook_callback, u);
> + u->card_profile_hook_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) card_profile_change_callback, u);
> + u->port_new_hook_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_PORT_ADDED], PA_HOOK_NORMAL, (pa_hook_cb_t) card_port_add_callback, u);
> + u->port_offset_hook_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_PORT_OFFSET_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) port_offset_change_callback, u);
> + u->hooks_connected = true;
>
> if (!(fname = pa_state_path("card-database", TRUE)))
> goto fail;
> @@ -433,9 +549,6 @@ int pa__init(pa_module*m) {
> pa_log_info("Successfully opened database file '%s'.", fname);
> pa_xfree(fname);
>
> - PA_IDXSET_FOREACH(card, m->core->cards, idx)
> - subscribe_callback(m->core, PA_SUBSCRIPTION_EVENT_CARD|PA_SUBSCRIPTION_EVENT_NEW, card->index, u);
This removal is maybe a bit questionable. But I think this is fine,
because module-card-restore should be always loaded before loading any
cards. I'm generally against having any limitations regarding module
loading order, but in this case it can't really be avoided: if
module-card-restore is loaded late, cards will be initialized with wrong
parameters. So, since we can expect that any sane configuration will
load module-card-restore before any cards exist, there's no need to do
anything with pre-existing cards.
--
Tanu
