[Asterisk-code-review] main/cdr: Allow modules to modify CDR fields before dispacth... (asterisk[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: main/cdr: Allow modules to modify CDR fields before dispacthing them
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

Thanks for the contribution!

I think this is a pretty interesting idea - the findings I have below are minor nitpicks and a request for some additional documentation in the header file about how the modification works.

https://gerrit.asterisk.org/#/c/1450/1/include/asterisk/cdr.h
File include/asterisk/cdr.h:

Line 543:  * Used to register a Call Detail Record modifier.
This is an interesting idea - I like the notion of giving the backends a chance to modify the CDR before it is dispatched.

I think it is worth noting in the description of this function that a CDR modifier modifies the CDR for *all* backends when the CDR is dispatched to the backends for writing. That is, if cdr_manager perturbs the CDR data, cdr_adaptive_odbc will also get the modified CDR.

That's clearly by design, but I think it should be noted somewhere in the description of the registration function.


https://gerrit.asterisk.org/#/c/1450/1/main/cdr.c
File main/cdr.c:

Line 2684: static int ast_cdr_generic_register(struct be_list *generic_list, const char *name, const char *desc, ast_cdrbe be)
Internal functions are generally not prefixed with ast_. I'd rename this to cdr_generic_register, since it is now static.


Line 2728: static int ast_cdr_generic_unregister(struct be_list *generic_list, const char *name)
Same finding about removing the ast_ applies here as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If11d8fd19ef89b1a66ecacf1201e10fcf86ccd56
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Jonh Wendell <jonh.wendell at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes