[Asterisk-code-review] main/pbx: Split pbx builtin functions to pbx builtins.c (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: main/pbx: Split pbx_builtin functions to pbx_builtins.c
......................................................................


Patch Set 3: Code-Review-1

(4 comments)

Thanks for getting this started George!

https://gerrit.asterisk.org/#/c/1878/3/main/pbx.c
File main/pbx.c:

Line 100: 	<application name="Answer" language="en_US">
Documentation should be moved with the dialplan apps.


Line 11284: 	res |= load_pbx_builtins(&globals, &globalslock);
I don't agree with the dual-ownership of globals/globalslock.  These variables should be private to a single source, never shared.

The easiest way to fix this is to move some already public functions back to main/pbx.c (at least for now):
pbx_builtin_getvar_helper
pbx_builtin_pushvar_helper
pbx_builtin_setvar_helper
pbx_builtin_clear_globals

Note: none of these functions are dialplan apps, these are actual API calls.  Moving them out of pbx.c would be nice but I think globals and globalslock need to stay with them.  That said maybe these functions belong in another source that would be dedicated to channel/global variable handling?  In either case I think these functions should be left alone for now, deal with them in another patch.

On another less important note I would prefer that load_pbx_builtins be called from asterisk.c after load_pbx.


https://gerrit.asterisk.org/#/c/1878/3/main/pbx_private.h
File main/pbx_private.h:

Line 32: void unload_pbx_builtins(void);
       : int load_pbx_builtins(struct varshead *g, ast_rwlock_t *l);
Can we move load_pbx_builtins to asterisk/_private.h and make unload_pbx_builtins static (use ast_register_cleanup from load_pbx_builtins)?


Line 34: int pbx_builtin_congestion(struct ast_channel *, const char *);
       : int pbx_builtin_busy(struct ast_channel *, const char *);
These symbols were previously static.  Can we remove the pbx_ prefix from these so they do not become exports from main/asterisk?

If we are going to create new export symbols I'd prefer they have a proper C API such as:
int ast_indicate_congestion(struct ast_channel *chan, double waitsec);
int ast_indicate_busy(struct ast_channel *chan, double waitsec);

In that case those functions would just be listed in a public header - maybe channel.h?


-- 
To view, visit https://gerrit.asterisk.org/1878
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I87066be3dbf7f5822942ac1449d98cc43fc7561a
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes