On 10/07/2013 05:39 PM, Arun Raghavan wrote:
> This allows us to parse an extra set of modargs to tack on to an
> existing set. Duplicates in the second set are ignored, since this fits
> our use best. In the future, this could be extended to support different
> merge modes (ignore dupes vs. replace with dupes), but I've left this
> out since there isn't a clear need and it would be dead code for now.
Fair point, but I think of an append function as replacing rather than
ignoring - maybe a better name would be pa_modargs_append_ignoredups()
or so?
> ---
> src/pulsecore/modargs.c | 54 ++++++++++++++++++++++++++++++++-----------------
> src/pulsecore/modargs.h | 2 ++
> 2 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/src/pulsecore/modargs.c b/src/pulsecore/modargs.c
> index 7665b91..ddaa54d 100644
> --- a/src/pulsecore/modargs.c
> +++ b/src/pulsecore/modargs.c
> @@ -45,7 +45,7 @@ struct entry {
> char *key, *value;
> };
>
> -static int add_key_value(pa_modargs *ma, char *key, char *value, const char* const valid_keys[]) {
> +static int add_key_value(pa_modargs *ma, char *key, char *value, const char* const valid_keys[], bool ignore_dupes) {
> struct entry *e;
> char *raw;
>
> @@ -58,7 +58,11 @@ static int add_key_value(pa_modargs *ma, char *key, char *value, const char* con
> if (pa_hashmap_get(ma->unescaped, key)) {
> pa_xfree(key);
> pa_xfree(value);
> - return -1;
> +
> + if (ignore_dupes)
> + return 0;
> + else
> + return -1;
> }
>
> if (valid_keys) {
> @@ -102,7 +106,7 @@ static void free_func(void *p) {
> pa_xfree(e);
> }
>
> -pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> +static int pa_modargs_parse(pa_modargs *ma, const char *args, const char* const* valid_keys, bool ignore_dupes) {
> enum {
> WHITESPACE,
> KEY,
> @@ -117,13 +121,6 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
>
> const char *p, *key = NULL, *value = NULL;
> size_t key_len = 0, value_len = 0;
> - pa_modargs *ma = pa_xnew(pa_modargs, 1);
> -
> - ma->raw = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, free_func);
> - ma->unescaped = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, free_func);
> -
> - if (!args)
> - return ma;
>
> state = WHITESPACE;
>
> @@ -162,7 +159,8 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> if (add_key_value(ma,
> pa_xstrndup(key, key_len),
> pa_xstrdup(""),
> - valid_keys) < 0)
> + valid_keys,
> + ignore_dupes) < 0)
> goto fail;
> state = WHITESPACE;
> } else if (*p == '\\') {
> @@ -181,7 +179,8 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> if (add_key_value(ma,
> pa_xstrndup(key, key_len),
> pa_xstrndup(value, value_len),
> - valid_keys) < 0)
> + valid_keys,
> + ignore_dupes) < 0)
> goto fail;
> state = WHITESPACE;
> } else if (*p == '\\') {
> @@ -201,7 +200,8 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> if (add_key_value(ma,
> pa_xstrndup(key, key_len),
> pa_xstrndup(value, value_len),
> - valid_keys) < 0)
> + valid_keys,
> + ignore_dupes) < 0)
> goto fail;
> state = WHITESPACE;
> } else if (*p == '\\') {
> @@ -221,7 +221,8 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> if (add_key_value(ma,
> pa_xstrndup(key, key_len),
> pa_xstrndup(value, value_len),
> - valid_keys) < 0)
> + valid_keys,
> + ignore_dupes) < 0)
> goto fail;
> state = WHITESPACE;
> } else if (*p == '\\') {
> @@ -239,23 +240,40 @@ pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> }
>
> if (state == VALUE_START) {
> - if (add_key_value(ma, pa_xstrndup(key, key_len), pa_xstrdup(""), valid_keys) < 0)
> + if (add_key_value(ma, pa_xstrndup(key, key_len), pa_xstrdup(""), valid_keys, ignore_dupes) < 0)
> goto fail;
> } else if (state == VALUE_SIMPLE) {
> - if (add_key_value(ma, pa_xstrndup(key, key_len), pa_xstrdup(value), valid_keys) < 0)
> + if (add_key_value(ma, pa_xstrndup(key, key_len), pa_xstrdup(value), valid_keys, ignore_dupes) < 0)
> goto fail;
> } else if (state != WHITESPACE)
> goto fail;
>
> - return ma;
> + return 0;
>
> fail:
> + return -1;
> +}
>
> - pa_modargs_free(ma);
> +pa_modargs *pa_modargs_new(const char *args, const char* const* valid_keys) {
> + pa_modargs *ma = pa_xnew(pa_modargs, 1);
> +
> + ma->raw = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, free_func);
> + ma->unescaped = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, free_func);
> +
> + if (args && pa_modargs_parse(ma, args, valid_keys, false) < 0)
> + goto fail;
>
> + return ma;
> +
> +fail:
> + pa_modargs_free(ma);
> return NULL;
> }
>
> +int pa_modargs_append(pa_modargs *ma, const char *args, const char* const* valid_keys) {
> + return pa_modargs_parse(ma, args, valid_keys, true);
> +}
> +
> void pa_modargs_free(pa_modargs*ma) {
> pa_assert(ma);
>
> diff --git a/src/pulsecore/modargs.h b/src/pulsecore/modargs.h
> index 95be54d..5637493 100644
> --- a/src/pulsecore/modargs.h
> +++ b/src/pulsecore/modargs.h
> @@ -35,6 +35,8 @@ typedef struct pa_modargs pa_modargs;
>
> /* Parse the string args. The NULL-terminated array keys contains all valid arguments. */
> pa_modargs *pa_modargs_new(const char *args, const char* const keys[]);
> +/* Parse the string args, and add any keys that are not already present. */
> +int pa_modargs_append(pa_modargs *ma, const char *args, const char* const* valid_keys);
It seems a bit inconsistent that modargs_new takes a "const char* const
keys[]" whereas pa_modargs_append takes a "const char* const*
valid_keys" (i e, one has an extra [] and the other an extra *).
Care to steer this up so it's the same everywhere?
Another option could be to store a pointer to valid_keys in
pa_modargs_new and just use that pointer in modargs_append. But I wonder
if we dynamically create this array somewhere and destroy it afterwards,
if so, it's not a good idea to store this pointer...
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
