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

[Alan Young <Alan.Young@IEE.org>]

On 17/02/17 16:53, Takashi Iwai wrote:
> 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.

Well, it does actually work in both cases but I guess that the plugin 
could get passed an unexpected type of compound config converter 
parameter in some cases.

>
> 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.
The following makes the difference more explicit What do you think?

     else if (snd_config_get_type(converter) == SND_CONFIG_TYPE_COMPOUND) {
         snd_config_iterator_t i, next;
         int pos = 0, is_array = 0;
         /*
          * If the convertor compound is an array of alternatives then 
the id of
          * the first element will be "0" (or maybe NULL). Otherwise 
assume it is
          * a structure and must have a "name" id to identify the 
converter type.
          */
         snd_config_for_each(i, next, converter) {
             snd_config_t *n = snd_config_iterator_entry(i);
             const char *id;
             snd_config_get_id(n, &id);
             if (pos++ == 0 && (!id || strcmp(id, "0") == 0))
                 is_array = 1;
             if (!is_array && strcmp(id, "name") != 0)
                 continue;
             if (snd_config_get_string(n, &type) < 0)
                 break;
             err = rate_open_func(rate, type, is_array ? NULL : 
converter, 0);
             if (!err || !is_array)
                 break;
         }

> 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?
>

Yes, left over from earlier version where I just changed the old func's 
signature.

Alan.