Re: [PATCH v2 07/13] topology: Add private data parser

[Liam Girdwood <liam.r.girdwood@linux.intel.com>]

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