On Thu, 2012-06-28 at 13:17 +0200, poljar (Damir Jeli?) wrote:
> From: "poljar (Damir Jeli?)" <poljarinho at gmail.com>
>
> module-card-restore now saves the latency offsets.
>
> This change includes a entry version bump.
>
> The entry now consists of a port count and a port name and offset for
> every port that belongs to the relevant card.
> ---
> src/modules/module-card-restore.c | 90 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/src/modules/module-card-restore.c b/src/modules/module-card-restore.c
> index e90e610..55589c8 100644
> --- a/src/modules/module-card-restore.c
> +++ b/src/modules/module-card-restore.c
> @@ -68,11 +68,14 @@ struct userdata {
> pa_database *database;
> };
>
> -#define ENTRY_VERSION 1
> +#define ENTRY_VERSION 2
>
> struct entry {
> uint8_t version;
> char *profile;
> + uint8_t port_count;
I'd go with uint32_t. It's unlikely that 8 bits wouldn't be enough, but
I think it's still somewhat possible that some card would some day have
more than 255 ports.
> + char **port_names;
> + int64_t *port_offsets;
I'd prefer one array of structs containing the port info, instead of
multiple arrays where each item contains one field of the port info.
> };
>
> static void save_time_callback(pa_mainloop_api*a, pa_time_event* e, const struct timeval *t, void *userdata) {
> @@ -99,14 +102,31 @@ static void trigger_save(struct userdata *u) {
>
> static struct entry* entry_new(void) {
> struct entry *r = pa_xnew0(struct entry, 1);
> +
> r->version = ENTRY_VERSION;
> + r->port_count = 0;
> + r->port_names = NULL;
> + r->port_offsets = NULL;
> +
These changes can be dropped, because the entry is already zeroed.
> return r;
> }
>
> static void entry_free(struct entry* e) {
> + int i;
I'd prefer unsigned for variables that are inherently unsigned in
nature, like array indexes. This is such a minor thing that if you are
at all against the change, don't do it.
> +
> pa_assert(e);
>
> pa_xfree(e->profile);
> +
> + if (e->port_count > 0)
> + for (i = 0; i < e->port_count; i++)
> + pa_xfree(e->port_names[i]);
> +
> + if (e->port_names)
> + pa_xfree(e->port_names);
> + if (e->port_offsets)
> + pa_xfree(e->port_offsets);
> +
> pa_xfree(e);
> }
>
> @@ -114,6 +134,7 @@ static pa_bool_t entry_write(struct userdata *u, const char *name, const struct
> pa_tagstruct *t;
> pa_datum key, data;
> pa_bool_t r;
> + int i;
"int" -> "unsigned"
>
> pa_assert(u);
> pa_assert(name);
> @@ -122,6 +143,12 @@ static pa_bool_t entry_write(struct userdata *u, const char *name, const struct
> t = pa_tagstruct_new(NULL, 0);
> pa_tagstruct_putu8(t, e->version);
> pa_tagstruct_puts(t, e->profile);
> + pa_tagstruct_putu8(t, e->port_count);
> +
> + for (i = 0; i < e->port_count; i++) {
> + pa_tagstruct_puts(t, e->port_names[i]);
> + pa_tagstruct_puts64(t, e->port_offsets[i]);
> + }
>
> key.data = (char *) name;
> key.size = strlen(name);
> @@ -177,6 +204,8 @@ static struct entry* entry_read(struct userdata *u, const char *name) {
> struct entry *e = NULL;
> pa_tagstruct *t = NULL;
> const char* profile;
> + const char *port_name = NULL;
> + int64_t port_offset = 0;
These could be declared later, because they are not needed at this
scope.
>
> pa_assert(u);
> pa_assert(name);
> @@ -201,6 +230,28 @@ static struct entry* entry_read(struct userdata *u, const char *name) {
>
> e->profile = pa_xstrdup(profile);
>
> + if (e->version >= 2) {
> + int i;
"int" -> "unsigned"
> +
> + pa_tagstruct_getu8(t, &e->port_count);
The return value must be checked.
> +
> + if (e->port_count > 0) {
> + e->port_names = pa_xnew(char*, e->port_count);
> + e->port_offsets = pa_xnew(int64_t, e->port_count);
> +
> + for (i = 0; i < e->port_count; i++) {
> + if (pa_tagstruct_gets(t, &port_name) < 0 ||
> + pa_tagstruct_gets64(t, &port_offset) < 0) {
> + e->port_count = i;
> + goto fail;
> + }
> +
> + e->port_names[i] = pa_xstrdup(port_name);
> + e->port_offsets[i] = port_offset;
> + }
> + }
> + }
> +
> if (!pa_tagstruct_eof(t))
> goto fail;
>
> @@ -237,6 +288,10 @@ fail:
> static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint32_t idx, void *userdata) {
> struct userdata *u = userdata;
> struct entry *entry, *old;
> + void *state;
Can be declared later.
> + int i = 0;
"int" -> "unsigned"
> + pa_bool_t dirty = false;
> + pa_device_port *p;
These can be declared later. Also, you should use "FALSE" instead of
"false". (We could start a discussion about deprecating pa_bool_t and
starting to use plain bool, though, since we are using other C99
features anyway, so it shouldn't be a compatibility issue. I'll do
that.)
> pa_card *card;
>
> pa_assert(c);
> @@ -255,9 +310,29 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
> entry = entry_new();
> entry->profile = pa_xstrdup(card->active_profile ? card->active_profile->name : "");
Before these lines there's this:
if (!card->save_profile)
return;
You need to do something about that. If save_profile is FALSE, you will
still want to update the database if the latency offsets change.
>
> + entry->port_count = pa_hashmap_size(card->ports);
> + if (entry->port_count > 0) {
> + entry->port_names = pa_xnew(char*, entry->port_count);
> + entry->port_offsets = pa_xnew(int64_t, entry->port_count);
> +
> + PA_HASHMAP_FOREACH(p, card->ports, state) {
> + entry->port_names[i] = pa_xstrdup(p->name);
> + entry->port_offsets[i] = p->latency_offset;
> + i++;
> + }
> + }
> +
> if ((old = entry_read(u, card->name))) {
>
> - if (pa_streq(old->profile, entry->profile)) {
> + if (pa_streq(old->profile, entry->profile) &&
> + old->port_count == entry->port_count)
> + for (i = 0; i < old->port_count; i++) {
> + if (!pa_streq(old->port_names[i], entry->port_names[i]) ||
> + old->port_offsets[i] != entry->port_offsets[i])
> + dirty = true;
> + }
This isn't right. dirty must be set to TRUE if the profile has changed
(and save_profile is TRUE) *or* if the offsets have changed. This code
doesn't do that.
> +
> + if(!dirty) {
Missing space after "if".
> entry_free(old);
> entry_free(entry);
> return;
Later in this function there's this:
pa_log_info("Storing profile for card %s.", card->name);
That's not accurate anymore after your changes.
> @@ -276,6 +351,8 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
>
> static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new_data, struct userdata *u) {
> struct entry *e;
> + int i;
> + pa_device_port *p;
Can be declared later.
>
> pa_assert(new_data);
>
> @@ -285,9 +362,18 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, pa_card_new_data *new
> pa_log_info("Restoring profile for card %s.", new_data->name);
> pa_card_new_data_set_profile(new_data, e->profile);
> new_data->save_profile = TRUE;
> +
> } else
> pa_log_debug("Not restoring profile for card %s, because already set.", new_data->name);
>
> + /* Always restore the latency offsets because their
> + * initial value is always 0 */
The if condition is:
if ((e = entry_read(u, new_data->name)) && e->profile[0]) {
If the e->profile[0] check fails, then the offsets won't get restored.
> + for (i = 0; i < e->port_count; i++) {
> + p = pa_hashmap_get(new_data->ports, e->port_names[i]);
> + if(p)
Missing space after "if".
--
Tanu
