[Asterisk-Dev] Misbehaviour in ast_variable_browse
Luis Vazquez
luis at teledata.com.uy
Tue Feb 22 03:48:34 MST 2005
Hello again Brian.
It seem now you have readed my email to the end ;)
Ok, you are right.
Anybody could do it that way, even that was the first way I solved the
problem ...
but I don't think that's good programming practice.
What if tomorrow anybody changes the way the variables list is
implemented? This way in the application I would be mixing the
application api with the private implementation code (v->next?)
If not what is the reason we have the current definition of
ast_category_browse() :
char *ast_category_browse(struct ast_config *config, char *prev)
to be used with this semantic:
cat = ast_category_browse(cfg, NULL);
while(cat) {
...
cat = ast_category_browse(cfg, cat);
}
to be consistent with the same approach you apply to
ast_variable_browse, the definition of ast_category_browse() should be:
struct ast_category *ast_category_browse(struct ast_config *config,
struct ast_category *prev)
an you could do this:
cat = ast_category_browse(cfg, NULL);
while(cat) {
/* do our thinng */
cat = cat->next;
}
So to resume my statement is not
"it is not possible to do this with the current implementation"
but
"I think the way it's implemented now is not correct by two reasons:
1) is not consistent con ast_category_browse()´s semantic
2) to solve a simple problem (traverse all the vars in a config
category) you must break the data encapsulation and depend on the
internal implementation of the var list.
Not a problem for me, only wanting to contribute to get a better
asterisk api even in a minor point like this.
Best regards,
Luis
Brian West wrote:
> Also on more thinking you can do this too(and reading this again):
>
> v = ast_variable_browse(cfg, "mystuff");
> while(v) {
> /* do our thinng */
> v = v->next;
> }
>
> You could also do:
>
> for (v = ast_variable_browse(config, "mystuff"); v; v = v->next) {
> /* do our thing here */
> }
>
> /b
>
> On Feb 21, 2005, at 7:20 PM, Brian West wrote:
>
>> I don't think this is needed because if you know the var you want why
>> not use ast_variable_retrieve to just get it.
>>
>> /b
>>
>> On Feb 21, 2005, at 12:29 PM, Luis Vazquez wrote:
>>
>>> Hello all,
>>> despite I have made many minor changes to asterisk code and some
>>> bugs reports and fixes this is my first mail to the dev list (I was
>>> only a "voyeur" here until now) so first of all I want to
>>> congratulate every one at the list for the great list it's and to
>>> thank a lot for the many things I have learned reading the mails here.
>>>
>>> I want to comment of what I consider is a inconsistency in the
>>> behavior of the method ast_variable_browse respect to the (suposedly
>>> analog, even said in the inline doc) ast_category_browse method in
>>> config.c.
>>>
>>> The point is that with ast_category_browse(config, prev) you can
>>> really iterate over the config sections with a code snippet like this:
>>>
>>> /************ works now****************/
>>> cat = ast_category_browse(cfg, NULL);
>>> while(cat) {
>>> .....
>>> cat = ast_category_browse(cfg, cat);
>>> }
>>> /************************************/
>>>
>>> So it would be logic to be able to do the same with
>>> ast_variable_browse, like this:
>>>
>>> /***** not posible now **************/
>>> var = ast_variable_browse(cfg, cat, NULL);
>>> while( var ) {
>>> .........
>>> var = ast_variable_browse(cfg, cat, var);
>>> }
>>> /*************************************/
>>>
>>> But this is not posible now because the actual prototype of
>>> ast_variable_browse() is:
>>>
>>> > struct ast_variable *ast_variable_browse(struct ast_config
>>> *config, char *category)
>>>
>>> and allways returns the root ast_variable in the given category,
>>> even when the doc text in the header file config.h suggests it
>>> should work as ast_category_browse.
>>> ...................................
>>> //! Goes through variables
>>> /*!
>>> * Somewhat similar in intent as the ast_category_browse.....
>>> */
>>> ..........................................
>>>
>>> The best solution I think would be to replace the prototype for
>>> something like this:
>>>
>>> > struct ast_variable *ast_variable_browse(struct ast_config
>>> *config, char *category, struct ast_variable *prev[=NULL])
>>>
>>> but because gcc (at least with the compile options used in asterisk)
>>> do not allow default param values it would require to change all
>>> places where ast_variable_browse() is used with the prior prototipe
>>> (and meaning) with an extra NULL parameter like this:
>>> - var = ast_variable_browse(config, cat);
>>> + var = ast_variable_browse(config, cat, NULL);
>>>
>>>
>>> So to avoid this odd changes what I have done is to add a new function:
>>>
>>> struct ast_variable *ast_variable_next(struct ast_config *config,
>>> char *category,struct ast_variable *prev);
>>>
>>> that returns the next ast_variable in a config category if prev is
>>> not null, and the root var (as the current ast_variable_browse) if
>>> prev==NULL.
>>>
>>> This approach allows to write code like this:
>>> /***************************************/
>>> var = ast_variable_browse(cfg, cat);
>>> while( var ) {
>>> ............
>>> var = ast_variable_next(cfg, cat, var);
>>> }
>>> /***************************************/
>>> without changing the places where ast_variable_browse() was used
>>> previously.
>>>
>>> This is needed when you want to retrieve all the vars in a section
>>> and don't know the variable names in advance.
>>> I need this because I'm using a custom asterisk application to load
>>> the configuration parameters used in the many pbx facility
>>> applications (mostly perl or c++ agi scripts) into the asterisk db;
>>> and I want to be able to add a new parameter without needing to
>>> recompilate the loader application (what I should do to use
>>> ast_variable_retrieve() because I need to know the variable names in
>>> advance)
>>>
>>> Attached is the code with the proposed new ast_variable_next()
>>> function.
>>>
>>> Best regards and I would like to receive any feedback and opinions
>>> about this suggestion.
>>> Luis
>>>
>>>
>>>
>>>
>>>
>>> --- config.c.ipc 2005-01-27 17:04:46.000000000 -0300
>>> +++ config.c 2005-02-21 16:18:06.990108528 -0300
>>> @@ -149,6 +149,24 @@
>>> return NULL;
>>> }
>>>
>>> +struct ast_variable *ast_variable_next(struct ast_config *config,
>>> char *category,struct ast_variable *prev)
>>> +{
>>> + struct ast_variable *v = NULL;
>>> + v = ast_variable_browse(config, category);
>>> + if(v) {
>>> + if(prev) {
>>> + while (v) {
>>> + if (v == prev)
>>> + return v->next;
>>> + v=v->next;
>>> + }
>>> + } else {
>>> + return v;
>>> + }
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> char *ast_variable_retrieve(struct ast_config *config, char
>>> *category, char *value)
>>> {
>>> struct ast_variable *v;
>>> --- include/asterisk/config.h.orig 2004-03-02 04:57:06.000000000
>>> -0300
>>> +++ include/asterisk/config.h 2005-02-21 16:13:08.389809756 -0300
>>> @@ -69,6 +69,14 @@
>>> */
>>> struct ast_variable *ast_variable_browse(struct ast_config *config,
>>> char *category);
>>>
>>> +//! Iterates through variables
>>> +/*!
>>> + * Somewhat similar in intent as the ast_category_browse. The prev
>>> variable MUST be an actual pointer to an actual variable (such as
>>> one obtained by using ast_variable_browse() or ast_variable_next()
>>> itself).
>>> + * List variables of config file
>>> + * Returns next variable on the list, or NULL on failure
>>> + */
>>> +struct ast_variable *ast_variable_next(struct ast_config *config,
>>> char *category,struct ast_variable *prev);
>>> +
>>> //! Gets a variable
>>> /*!
>>> * \param config which (opened) config to use
>>> _______________________________________________
>>> Asterisk-Dev mailing list
>>> Asterisk-Dev at lists.digium.com
>>> http://lists.digium.com/mailman/listinfo/asterisk-dev
>>> To UNSUBSCRIBE or update options visit:
>>> http://lists.digium.com/mailman/listinfo/asterisk-dev
>>
>>
>> _______________________________________________
>> Asterisk-Dev mailing list
>> Asterisk-Dev at lists.digium.com
>> http://lists.digium.com/mailman/listinfo/asterisk-dev
>> To UNSUBSCRIBE or update options visit:
>> http://lists.digium.com/mailman/listinfo/asterisk-dev
>
>
> _______________________________________________
> Asterisk-Dev mailing list
> Asterisk-Dev at lists.digium.com
> http://lists.digium.com/mailman/listinfo/asterisk-dev
> To UNSUBSCRIBE or update options visit:
> http://lists.digium.com/mailman/listinfo/asterisk-dev
>
More information about the asterisk-dev
mailing list