Re: [PATCH] [updated] pcm: rate: Add capability to pass configuration node to plugins

[Takashi Iwai <tiwai@suse.de>]

On Wed, 15 Feb 2017 13:28:38 +0100,
Alan Young wrote:
> 
> On 14/02/17 07:30, Alan Young wrote:
> > However, looking at that again now, I see that the test is flawed
> > because it has no marker to check that it is only applied on the
> > first iteration, so the loop would always stop after testing the
> > second alternative. I could fix that or remove the test altogether -
> > what do you think?
> 
> Here is an updated patch with a more robust check.

I still doubt whether it works as you expected...

>  	else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
>  		snd_config_iterator_t i, next;
>  		snd_config_for_each(i, next, converter) {
>  			snd_config_t *n = snd_config_iterator_entry(i);
> +			const char *id;
>  			if (snd_config_get_string(n, &type) < 0)
>  				break;
> -			err = rate_open_func(rate, type, 0);
> +			err = rate_open_func(rate, type, converter, 0);
>  			if (!err)
>  				break;
> +			if (snd_config_get_id(n, &id) >= 0 && id && !isdigit(*id))
> +				break; /* not a simple array of types */
>  		}

The problem is that you pass always the compound object as the
converter argument.  As you already suggested, there are two cases:
one is a string array and another is a compound with optional args.
In your code, the first iteration doesn't check which type is.  So it
always fails if the string array is passed.

That is, the right implementation is to check whether the given
compound is a string array is.  If yes, it goes to the old style
loop (and you can check "name" argument properly).  If not, it goes
with the new compound argument.  That's simple enough, and more
understandable the condition you used for the loop termination. 

BTW, one another thing:
> @@ -1386,7 +1414,7 @@ int snd_pcm_rate_open(snd_pcm_t **pcmp, const char *name,
>  #else
>  	type = "linear";
>  	open_func = SND_PCM_RATE_PLUGIN_ENTRY(linear);
> -	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops);
> +	err = open_func(SND_PCM_RATE_PLUGIN_VERSION, &rate->obj, &rate->ops, 0);

Is this really correct?  I thought SND_PCM_RATE_PLUGIN_ENTRY() refers
to the old style?


thanks,

Takashi