Summary:ASTERISK-04070: [patch] macros for simpler string handling
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-05-04 13:55:51Date Closed:2005-06-05 22:30:38
Versions:Frequency of
Environment:Attachments:( 0) macros.diff
Description:I am attaching a patch to include/asterisk/utils.h and asterisk.c
(meant as an example of use) with some macros to provide more readable and
correct argument.
Some of the macros (AP_START, AP_F, AP_*) are meant to simplify the writing
of switch statements involving strings, eg. when parsing lines of the
form token - value in config files.
Others, AP_S_OVERWRITE etc are meant to simplify the overwriting of
statically and dynamically allocated strings with new values, by taking
care of null pointer dereferences, size constraints, deallocation of previous
If the form is acceptable the same 'cure' could be applied to many other files.


disclaimer already sent in.
Comments:By: Kevin P. Fleming (kpfleming) 2005-05-04 15:59:14

If anything like this is going to be done, it would make more sense to declare a set of structures defining the possible arguments and their targets, and then call an API function to process the possibilities against what has been loaded from the config.

Personally, I don't see these macros as enough of a benefit to be worth the change.

By: Luigi Rizzo (rizzo) 2005-05-05 02:06:09

your comment seems to address the argument parsing macros.
Regarding this,
I thought about implementing what you say, but it has two practical problems:
1) many times you have to use macros anyways to initialize the struct,
  otherwise you have to repeat the argument, as in
       {"astetcdir", STATIC_STRING,  ast_config_AST_CONFIG_DIR,
                 sizeof(ast_config_AST_CONFIG_DIR) },
2) a few times, at least at the moment, the handler is more complex than
  simply setting a variable, and requires a block of code which is very
  specific, acts on several variables, etc., so defining a handler for
  these cases would be rather innatural.
While I believe instances of the second class can be largely removed
over time, the first problem still remains.

My view is that macros of this kind have the following advantages:
-  reduce the chance of errors in writing the argument parsing code -- it
  is very easy, in sequences like strncpy(dst, src, sizeof(foo) -1)
  to put the wrong argument in sizeof() as i did above, or to forget
  the -1 as it is done in asterisk.c, channel.c, dlfcn.c, pbx.c and
  over 35 times in say.c
-  make argument processing more consistent - people will not try to write
  their own code to parse an int, uint, ip address or string, but
  instead use one of the supported methods.
-  make code more readable because in most cases a block of 3-4 lines is
  replaced by a single short line, and this in sections of code where
  you have to match several keywords.

By: Luigi Rizzo (rizzo) 2005-05-05 02:08:14

and to comment the string handling macros, AP_S_OVERWRITE, AP_D_OVERWRITE
and AP_S_COPY, there is no need in these cases to go through structures
and API functions, and all the advantages i mentioned in the previous
message still apply.

By: Kevin P. Fleming (kpfleming) 2005-05-15 02:18:41

I have a replacement config parser patch in the works (currently being reviewed by Mark and others outside of Mantis), that implements hashtable-based matching of configuration item names and related bits. It is structured in such a way that it could be extended to provide value parsing as well, validation (range checking, etc.) and more when we find it useful.

Given that, it's likely that the immediate need for your changes (config file parsing in modules) will go away soon, as the code in those modules will change in a very different way.

Your AP_S_OVERWRITE macro claims to ensure that the destination buffer will be null-terminated, but since it is relying on strncpy(), that will not be true if the buffer does not already end with a null character.

In general, I don't see a lot of value in adding macros to overlay such simple operations (in the case of string copying, not argument parsing), but I'm willing to be convinced.

By: Olle Johansson (oej) 2005-06-05 17:03:07

kpfleming: Time to close this since it doesn't seem approved for CVS?


By: Kevin P. Fleming (kpfleming) 2005-06-05 22:30:10

Yes, the initial hashtable config parser is now running on the Iaxtel server and performing quite well, so once it is in CVS we can both extend it to other modules (I've only done chan_sip and chan_iax2), and also extend it to understanding argument types and actually parsing/validating them.