Summary:	ASTERISK-04070: [patch] macros for simpler string handling
Reporter:	Luigi Rizzo (rizzo)	Labels:
Date Opened:	2005-05-04 13:55:51	Date Closed:	2005-06-05 22:30:38
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
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 values. If the form is acceptable the same 'cure' could be applied to many other files. ****** ADDITIONAL INFORMATION ****** 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? /Housekeeping 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.