| Message ID | 1435758275-4047-7-git-send-email-liam.r.girdwood@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Wed, 1 Jul 2015 14:44:29 +0100, Liam Girdwood wrote: > > +static int tplg_parse_data_file(snd_config_t *cfg, struct tplg_elem *elem) > +{ > + struct snd_soc_tplg_private *priv = NULL; > + const char *value = NULL; > + char filename[MAX_FILE]; > + char *env = getenv(ALSA_CONFIG_TPLG_VAR); > + FILE *fp; > + size_t size, bytes_read; > + int ret = 0; > + > + tplg_dbg("data DataFile: %s\n", elem->id); > + > + if (snd_config_get_string(cfg, &value) < 0) > + return -EINVAL; > + > + /* prepend alsa config directory to path */ > + snprintf(filename, sizeof(filename), "%s/%s", > + env ? env : ALSA_TPLG_DIR, value); > + filename[sizeof(filename)-1] = '\0'; Unlike strncpy(), snprintf() puts the NUL-character by itself, so this is superfluous. > +static int get_hex_num(const char *str) > +{ > + char *tmp, *s = NULL; > + int i = 0; > + > + tmp = strdup(str); > + if (tmp == NULL) > + return -ENOMEM; > + > + s = strtok(tmp, ","); > + while (s != NULL) { > + s = strtok(NULL, ","); > + i++; > + } > + > + free(tmp); > + return i; Hmm, this just counts the number of comma + 1, so you don't need to duplicate the string? Takashi
On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: > > > +static int get_hex_num(const char *str) > > +{ > > + char *tmp, *s = NULL; > > + int i = 0; > > + > > + tmp = strdup(str); > > + if (tmp == NULL) > > + return -ENOMEM; > > + > > + s = strtok(tmp, ","); > > + while (s != NULL) { > > + s = strtok(NULL, ","); > > + i++; > > + } > > + > > + free(tmp); > > + return i; > > Hmm, this just counts the number of comma + 1, so you don't need to > duplicate the string? > The string here is duplicated since strtok is destructive (it overwrites the delimiters with NULL) and the string is being used later on by the calling function. Liam
On Tue, 07 Jul 2015 17:54:02 +0200, Liam Girdwood wrote: > > On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: > > > > > > +static int get_hex_num(const char *str) > > > +{ > > > + char *tmp, *s = NULL; > > > + int i = 0; > > > + > > > + tmp = strdup(str); > > > + if (tmp == NULL) > > > + return -ENOMEM; > > > + > > > + s = strtok(tmp, ","); > > > + while (s != NULL) { > > > + s = strtok(NULL, ","); > > > + i++; > > > + } > > > + > > > + free(tmp); > > > + return i; > > > > Hmm, this just counts the number of comma + 1, so you don't need to > > duplicate the string? > > > > The string here is duplicated since strtok is destructive (it overwrites > the delimiters with NULL) and the string is being used later on by the > calling function. Yes, but what I meant is something like below: static int get_hex_num(const char *str) { int i = 0; if (!*str) return 0; for (;;) { i++; str = strchr(str, ','); if (!str) return i; str++; } } ... so that it works without strdup(). But it seems that strtok() skips the starting delimiter or handles multiple delimiters as a single, so the result will become inconsistent. That is, all the following strings will give "a" and "b" via strtok(): a,b a,,b ,a,b a,b, I guess you don't want to have an empty list element, right? Takashi
On Tue, 2015-07-07 at 18:19 +0200, Takashi Iwai wrote: > On Tue, 07 Jul 2015 17:54:02 +0200, > Liam Girdwood wrote: > > > > On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: > > > > > > > > > +static int get_hex_num(const char *str) > > > > +{ > > > > + char *tmp, *s = NULL; > > > > + int i = 0; > > > > + > > > > + tmp = strdup(str); > > > > + if (tmp == NULL) > > > > + return -ENOMEM; > > > > + > > > > + s = strtok(tmp, ","); > > > > + while (s != NULL) { > > > > + s = strtok(NULL, ","); > > > > + i++; > > > > + } > > > > + > > > > + free(tmp); > > > > + return i; > > > > > > Hmm, this just counts the number of comma + 1, so you don't need to > > > duplicate the string? > > > > > > > The string here is duplicated since strtok is destructive (it overwrites > > the delimiters with NULL) and the string is being used later on by the > > calling function. > > Yes, but what I meant is something like below: > > static int get_hex_num(const char *str) > { > int i = 0; > > if (!*str) > return 0; > for (;;) { > i++; > str = strchr(str, ','); > if (!str) > return i; > str++; > } > } > > ... so that it works without strdup(). > > But it seems that strtok() skips the starting delimiter or handles > multiple delimiters as a single, so the result will become > inconsistent. That is, all the following strings will give "a" and > "b" via strtok(): > a,b > a,,b > ,a,b > a,b, > > I guess you don't want to have an empty list element, right? > Lets ask the author :) but IMO an empty list should be skipped here. Yao, what's your rational behind this code ? Liam
On 2015/7/8 16:57, Liam Girdwood wrote: > On Tue, 2015-07-07 at 18:19 +0200, Takashi Iwai wrote: >> On Tue, 07 Jul 2015 17:54:02 +0200, >> Liam Girdwood wrote: >>> >>> On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: >>> >>>> >>>>> +static int get_hex_num(const char *str) >>>>> +{ >>>>> + char *tmp, *s = NULL; >>>>> + int i = 0; >>>>> + >>>>> + tmp = strdup(str); >>>>> + if (tmp == NULL) >>>>> + return -ENOMEM; >>>>> + >>>>> + s = strtok(tmp, ","); >>>>> + while (s != NULL) { >>>>> + s = strtok(NULL, ","); >>>>> + i++; >>>>> + } >>>>> + >>>>> + free(tmp); >>>>> + return i; >>>> >>>> Hmm, this just counts the number of comma + 1, so you don't need to >>>> duplicate the string? >>>> >>> >>> The string here is duplicated since strtok is destructive (it overwrites >>> the delimiters with NULL) and the string is being used later on by the >>> calling function. >> >> Yes, but what I meant is something like below: >> >> static int get_hex_num(const char *str) >> { >> int i = 0; >> >> if (!*str) >> return 0; >> for (;;) { >> i++; >> str = strchr(str, ','); >> if (!str) >> return i; >> str++; >> } >> } >> >> ... so that it works without strdup(). >> >> But it seems that strtok() skips the starting delimiter or handles >> multiple delimiters as a single, so the result will become >> inconsistent. That is, all the following strings will give "a" and >> "b" via strtok(): >> a,b >> a,,b >> ,a,b >> a,b, >> >> I guess you don't want to have an empty list element, right? >> > > Lets ask the author :) but IMO an empty list should be skipped here. > > Yao, what's your rational behind this code ? Sorry for replying late, I just see this mail. The get_hex_num() returns the number of hexadecimal in string. Say the string is "0x12,0x34,0x56,0x78" or "0x12,0x34,0x56,0x78,", the get_hex_num() returns 4. But if I use strchr, for above string, I get different number (3 or 4). That's the reason I choose the strtok(). I do this is because I think user may append the comma at the end of string. > > Liam >
On Wed, 08 Jul 2015 15:31:27 +0200, Jin, Yao wrote: > > > > On 2015/7/8 16:57, Liam Girdwood wrote: > > On Tue, 2015-07-07 at 18:19 +0200, Takashi Iwai wrote: > >> On Tue, 07 Jul 2015 17:54:02 +0200, > >> Liam Girdwood wrote: > >>> > >>> On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: > >>> > >>>> > >>>>> +static int get_hex_num(const char *str) > >>>>> +{ > >>>>> + char *tmp, *s = NULL; > >>>>> + int i = 0; > >>>>> + > >>>>> + tmp = strdup(str); > >>>>> + if (tmp == NULL) > >>>>> + return -ENOMEM; > >>>>> + > >>>>> + s = strtok(tmp, ","); > >>>>> + while (s != NULL) { > >>>>> + s = strtok(NULL, ","); > >>>>> + i++; > >>>>> + } > >>>>> + > >>>>> + free(tmp); > >>>>> + return i; > >>>> > >>>> Hmm, this just counts the number of comma + 1, so you don't need to > >>>> duplicate the string? > >>>> > >>> > >>> The string here is duplicated since strtok is destructive (it overwrites > >>> the delimiters with NULL) and the string is being used later on by the > >>> calling function. > >> > >> Yes, but what I meant is something like below: > >> > >> static int get_hex_num(const char *str) > >> { > >> int i = 0; > >> > >> if (!*str) > >> return 0; > >> for (;;) { > >> i++; > >> str = strchr(str, ','); > >> if (!str) > >> return i; > >> str++; > >> } > >> } > >> > >> ... so that it works without strdup(). > >> > >> But it seems that strtok() skips the starting delimiter or handles > >> multiple delimiters as a single, so the result will become > >> inconsistent. That is, all the following strings will give "a" and > >> "b" via strtok(): > >> a,b > >> a,,b > >> ,a,b > >> a,b, > >> > >> I guess you don't want to have an empty list element, right? > >> > > > > Lets ask the author :) but IMO an empty list should be skipped here. > > > > Yao, what's your rational behind this code ? > > Sorry for replying late, I just see this mail. > > The get_hex_num() returns the number of hexadecimal in string. > > Say the string is "0x12,0x34,0x56,0x78" or "0x12,0x34,0x56,0x78,", > the get_hex_num() returns 4. > > But if I use strchr, for above string, I get different number (3 or 4). > That's the reason I choose the strtok(). > > I do this is because I think user may append the comma at the end of > string. So, is this allowed intentionally as a valid syntax? As I showed, a string like ",,,0x12,,0x34,,,,0x56,0x78,," would be handled as a valid string. The above may look strange, but the strtok() behavior is natural if you imagine to replace "," with a space. Takashi
On Wed, 2015-07-08 at 16:14 +0200, Takashi Iwai wrote: > On Wed, 08 Jul 2015 15:31:27 +0200, > Jin, Yao wrote: > > > > > > > > On 2015/7/8 16:57, Liam Girdwood wrote: > > > On Tue, 2015-07-07 at 18:19 +0200, Takashi Iwai wrote: > > >> On Tue, 07 Jul 2015 17:54:02 +0200, > > >> Liam Girdwood wrote: > > >>> > > >>> On Wed, 2015-07-01 at 18:20 +0200, Takashi Iwai wrote: > > >>> > > >>>> > > >>>>> +static int get_hex_num(const char *str) > > >>>>> +{ > > >>>>> + char *tmp, *s = NULL; > > >>>>> + int i = 0; > > >>>>> + > > >>>>> + tmp = strdup(str); > > >>>>> + if (tmp == NULL) > > >>>>> + return -ENOMEM; > > >>>>> + > > >>>>> + s = strtok(tmp, ","); > > >>>>> + while (s != NULL) { > > >>>>> + s = strtok(NULL, ","); > > >>>>> + i++; > > >>>>> + } > > >>>>> + > > >>>>> + free(tmp); > > >>>>> + return i; > > >>>> > > >>>> Hmm, this just counts the number of comma + 1, so you don't need to > > >>>> duplicate the string? > > >>>> > > >>> > > >>> The string here is duplicated since strtok is destructive (it overwrites > > >>> the delimiters with NULL) and the string is being used later on by the > > >>> calling function. > > >> > > >> Yes, but what I meant is something like below: > > >> > > >> static int get_hex_num(const char *str) > > >> { > > >> int i = 0; > > >> > > >> if (!*str) > > >> return 0; > > >> for (;;) { > > >> i++; > > >> str = strchr(str, ','); > > >> if (!str) > > >> return i; > > >> str++; > > >> } > > >> } > > >> > > >> ... so that it works without strdup(). > > >> > > >> But it seems that strtok() skips the starting delimiter or handles > > >> multiple delimiters as a single, so the result will become > > >> inconsistent. That is, all the following strings will give "a" and > > >> "b" via strtok(): > > >> a,b > > >> a,,b > > >> ,a,b > > >> a,b, > > >> > > >> I guess you don't want to have an empty list element, right? > > >> > > > > > > Lets ask the author :) but IMO an empty list should be skipped here. > > > > > > Yao, what's your rational behind this code ? > > > > Sorry for replying late, I just see this mail. > > > > The get_hex_num() returns the number of hexadecimal in string. > > > > Say the string is "0x12,0x34,0x56,0x78" or "0x12,0x34,0x56,0x78,", > > the get_hex_num() returns 4. > > > > But if I use strchr, for above string, I get different number (3 or 4). > > That's the reason I choose the strtok(). > > > > I do this is because I think user may append the comma at the end of > > string. > Ok, lets make commas at the end illegal. > So, is this allowed intentionally as a valid syntax? As I showed, > a string like ",,,0x12,,0x34,,,,0x56,0x78,," would be handled as a > valid string. > > The above may look strange, but the strtok() behavior is natural if > you imagine to replace "," with a space. > So I'll fix this to be "0x01, 0x02, 0x03" syntax only. i.e. comma between each value and no comma at the end. Liam
diff --git a/src/topology/data.c b/src/topology/data.c new file mode 100644 index 0000000..7de62b4 --- /dev/null +++ b/src/topology/data.c @@ -0,0 +1,358 @@ +/* + Copyright(c) 2014-2015 Intel Corporation + All rights reserved. + + This program is free software; you can redistribute it and/or modify + it under the terms of version 2 of the GNU General Public License as + published by the Free Software Foundation. + + This program 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. + + Authors: Mengdong Lin <mengdong.lin@intel.com> + Yao Jin <yao.jin@intel.com> + Liam Girdwood <liam.r.girdwood@linux.intel.com> +*/ + +#include "list.h" +#include "tplg_local.h" + +/* Get Private data from a file. */ +static int tplg_parse_data_file(snd_config_t *cfg, struct tplg_elem *elem) +{ + struct snd_soc_tplg_private *priv = NULL; + const char *value = NULL; + char filename[MAX_FILE]; + char *env = getenv(ALSA_CONFIG_TPLG_VAR); + FILE *fp; + size_t size, bytes_read; + int ret = 0; + + tplg_dbg("data DataFile: %s\n", elem->id); + + if (snd_config_get_string(cfg, &value) < 0) + return -EINVAL; + + /* prepend alsa config directory to path */ + snprintf(filename, sizeof(filename), "%s/%s", + env ? env : ALSA_TPLG_DIR, value); + filename[sizeof(filename)-1] = '\0'; + + fp = fopen(filename, "r"); + if (fp == NULL) { + fprintf(stderr, "error: invalid data file path '%s'\n", + filename); + ret = -errno; + goto err; + } + + fseek(fp, 0L, SEEK_END); + size = ftell(fp); + fseek(fp, 0L, SEEK_SET); + if (size <= 0) { + fprintf(stderr, "error: invalid data file size %zu\n", size); + ret = -EINVAL; + goto err; + } + if (size > TPLG_MAX_PRIV_SIZE) { + fprintf(stderr, "error: data file too big %zu\n", size); + ret = -EINVAL; + goto err; + } + + priv = calloc(1, sizeof(*priv) + size); + if (!priv) { + ret = -ENOMEM; + goto err; + } + + bytes_read = fread(&priv->data, 1, size, fp); + if (bytes_read != size) { + ret = -errno; + goto err; + } + + elem->data = priv; + priv->size = size; + elem->size = sizeof(*priv) + size; + return 0; + +err: + if (priv) + free(priv); + return ret; +} + +static void dump_priv_data(struct tplg_elem *elem) +{ + struct snd_soc_tplg_private *priv = elem->data; + unsigned char *p = (unsigned char *)priv->data; + unsigned int i, j = 0; + + tplg_dbg(" elem size = %d, priv data size = %d\n", + elem->size, priv->size); + + for (i = 0; i < priv->size; i++) { + if (j++ % 8 == 0) + tplg_dbg("\n"); + + tplg_dbg(" 0x%x", *p++); + } + + tplg_dbg("\n\n"); +} + +static int get_hex_num(const char *str) +{ + char *tmp, *s = NULL; + int i = 0; + + tmp = strdup(str); + if (tmp == NULL) + return -ENOMEM; + + s = strtok(tmp, ","); + while (s != NULL) { + s = strtok(NULL, ","); + i++; + } + + free(tmp); + return i; +} + +static int write_hex(char *buf, char *str, int width) +{ + long val; + void *p = &val; + + errno = 0; + val = strtol(str, NULL, 16); + + if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN)) + || (errno != 0 && val == 0)) { + return -EINVAL; + } + + switch (width) { + case 1: + *(unsigned char *)buf = *(unsigned char *)p; + break; + case 2: + *(unsigned short *)buf = *(unsigned short *)p; + break; + case 4: + *(unsigned int *)buf = *(unsigned int *)p; + break; + default: + return -EINVAL; + } + + return 0; +} + +static int copy_data_hex(char *data, int off, const char *str, int width) +{ + char *tmp, *s = NULL, *p = data; + int ret; + + tmp = strdup(str); + if (tmp == NULL) + return -ENOMEM; + + p += off; + s = strtok(tmp, ","); + + while (s != NULL) { + ret = write_hex(p, s, width); + if (ret < 0) { + free(tmp); + return ret; + } + + s = strtok(NULL, ","); + p += width; + } + + free(tmp); + return 0; +} + +static int tplg_parse_data_hex(snd_config_t *cfg, struct tplg_elem *elem, + int width) +{ + struct snd_soc_tplg_private *priv; + const char *value = NULL; + int size, esize, off, num; + int ret; + + tplg_dbg(" data: %s\n", elem->id); + + if (snd_config_get_string(cfg, &value) < 0) + return -EINVAL; + + num = get_hex_num(value); + size = num * width; + priv = elem->data; + + if (esize > TPLG_MAX_PRIV_SIZE) { + fprintf(stderr, "error: data too big %d\n", esize); + return -EINVAL; + } + + if (priv != NULL) { + off = priv->size; + esize = elem->size + size; + priv = realloc(priv, esize); + } else { + off = 0; + esize = sizeof(*priv) + size; + priv = calloc(1, esize); + } + + if (!priv) + return -ENOMEM; + + elem->data = priv; + priv->size += size; + elem->size = esize; + + ret = copy_data_hex(priv->data, off, value, width); + + dump_priv_data(elem); + return ret; +} + + +/* Parse Private data. + * + * Object private data can either be from file or defined as bytes, shorts, + * words. + * + * SectionData."data name" { + * + * DataFile "filename" + * bytes "0x12,0x34,0x56,0x78" + * shorts "0x1122,0x3344,0x5566,0x7788" + * words "0xaabbccdd,0x11223344,0x66aa77bb,0xefef1234" + * } + */ +int tplg_parse_data(snd_tplg_t *tplg, snd_config_t *cfg, + void *private ATTRIBUTE_UNUSED) +{ + snd_config_iterator_t i, next; + snd_config_t *n; + const char *id; + int err = 0; + struct tplg_elem *elem; + + elem = tplg_elem_new_common(tplg, cfg, PARSER_TYPE_DATA); + if (!elem) + return -ENOMEM; + + snd_config_for_each(i, next, cfg) { + + n = snd_config_iterator_entry(i); + if (snd_config_get_id(n, &id) < 0) { + continue; + } + + if (strcmp(id, "file") == 0) { + err = tplg_parse_data_file(n, elem); + if (err < 0) { + fprintf(stderr, "error: failed to parse data file\n"); + return err; + } + continue; + } + + if (strcmp(id, "bytes") == 0) { + err = tplg_parse_data_hex(n, elem, 1); + if (err < 0) { + fprintf(stderr, "error: failed to parse data bytes\n"); + return err; + } + continue; + } + + if (strcmp(id, "shorts") == 0) { + err = tplg_parse_data_hex(n, elem, 2); + if (err < 0) { + fprintf(stderr, "error: failed to parse data shorts\n"); + return err; + } + continue; + } + + if (strcmp(id, "words") == 0) { + err = tplg_parse_data_hex(n, elem, 4); + if (err < 0) { + fprintf(stderr, "error: failed to parse data words\n"); + return err; + } + continue; + } + } + + return err; +} + +/* copy private data into the bytes extended control */ +int tplg_copy_data(struct tplg_elem *elem, struct tplg_elem *ref) +{ + struct snd_soc_tplg_private *priv; + int priv_data_size; + + if (!ref) + return -EINVAL; + + tplg_dbg("Data '%s' used by '%s'\n", ref->id, elem->id); + priv_data_size = ref->data->size; + + switch (elem->type) { + case PARSER_TYPE_MIXER: + elem->mixer_ctrl = realloc(elem->mixer_ctrl, + elem->size + priv_data_size); + if (!elem->mixer_ctrl) + return -ENOMEM; + priv = &elem->mixer_ctrl->priv; + break; + + case PARSER_TYPE_ENUM: + elem->enum_ctrl = realloc(elem->enum_ctrl, + elem->size + priv_data_size); + if (!elem->enum_ctrl) + return -ENOMEM; + priv = &elem->enum_ctrl->priv; + break; + + case PARSER_TYPE_BYTES: + elem->bytes_ext = realloc(elem->bytes_ext, + elem->size + priv_data_size); + if (!elem->bytes_ext) + return -ENOMEM; + priv = &elem->bytes_ext->priv; + break; + + + case PARSER_TYPE_DAPM_WIDGET: + elem->widget = realloc(elem->widget, + elem->size + priv_data_size); + if (!elem->widget) + return -ENOMEM; + priv = &elem->widget->priv; + break; + + default: + fprintf(stderr, "elem '%s': type %d shall not have private data\n", + elem->id, elem->type); + return -EINVAL; + } + + elem->size += priv_data_size; + priv->size = priv_data_size; + memcpy(priv->data, ref->data->data, priv_data_size); + return 0; +}
Parse private data and store for attachment to other objects. Data can come file or be locally defined as bytes, shorts or words. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> --- src/topology/data.c | 358 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 358 insertions(+) create mode 100644 src/topology/data.c