[asterisk-dev] [Code Review] ast_callerid restructuring
Mark Michelson
mmichelson at digium.com
Mon Jun 14 18:04:48 CDT 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/702/#review2200
-----------------------------------------------------------
Just a pass through channel.[hc]. I'll look at the rest on another occasion.
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/702/#comment4663>
"tolerate"
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/702/#comment4661>
This function seems to do the same thing as ast_party_name_set(). Since ast_party_name_set has an extra safeguard in it, I think this function can be removed.
Of course, if there's something subtle I'm missing, I would suggest adding comments to the doxygen to clarify the differences between the functions.
This same principal applies to ast_party_number_set() and ast_party_number_copy(); ast_party_dialed_set() and ast_party_dialed_copy(); and ast_party_caller_set() and ast_party_caller_copy().
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4660>
I may have a misunderstanding of the valid flag. I'm not sure why you would copy the valid flag from the guide but set init->str to NULL. It seems like if you are going to unconditionally set the string NULL then the valid flag should be 0. Am I misunderstanding something here?
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4664>
Same comment about the valid flag as with ast_party_name_set_init()
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4667>
Since this is an internal function, remove the ast_ from the beginning of the function name, since that is used to indicate a function as being public.
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4668>
Remove ast_ from the function name.
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4669>
Remove ast_ from function name.
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/702/#comment4670>
Remove ast_ from function name.
- Mark
On 2010-06-10 18:24:42, rmudgett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/702/
> -----------------------------------------------------------
>
> (Updated 2010-06-10 18:24:42)
>
>
> Review request for Asterisk Developers and Mark Michelson.
>
>
> Summary
> -------
>
> The purpose of this patch is to eliminate struct ast_callerid since it has
> turned into a miscellaneous collection of various party information.
>
> Eliminate struct ast_callerid and replace it with the following struct
> organization:
>
> struct ast_party_name {
> char *str;
> int char_set;
> int presentation;
> unsigned char valid;
> };
> struct ast_party_number {
> char *str;
> int plan;
> int presentation;
> unsigned char valid;
> };
> struct ast_party_subaddress {
> char *str;
> int type;
> unsigned char odd_even_indicator;
> unsigned char valid;
> };
> struct ast_party_id {
> struct ast_party_name name;
> struct ast_party_number number;
> struct ast_party_subaddress subaddress;
> char *tag;
> };
> struct ast_party_dialed {
> struct {
> char *str;
> int plan;
> } number;
> struct ast_party_subaddress subaddress;
> int transit_network_select;
> };
> struct ast_party_caller {
> struct ast_party_id id;
> char *ani;
> int ani2;
> };
>
> Note that the "XXX_" prepended to the name and number members in
> ast_party_id in the diff are to ensure that all code has been converted
> and will be renamed when committed.
>
>
> The new organization adds some new information as well.
>
> * The party name and number now have their own presentation value that can
> be manipulated independently. ISDN supplies the presentation value for
> the name and number at different times with the possibility that they
> could be different.
>
> * The party name and number now have a valid flag. Before this change the
> name or number string could be empty if the presentation were restricted.
> Most channel drivers assume that the name or number is then simply not
> available instead of indicating that the name or number was restricted.
>
> * The party name now has a character set value. SIP and Q.SIG have the
> ability to indicate what character set a name string is using so it could
> be presented properly.
>
> * The dialed party now has a numbering plan value that could be useful to
> have available.
>
> The various channel drivers will need to be updated to support the new
> core features as needed. They have simply been converted to supply
> current functionality at this time.
>
>
> The following items of note were either corrected or enhanced:
>
> * The CONNECTEDLINE() and REDIRECTING() dialplan functions were
> consolidated into func_callerid.c to share party id handling code.
>
> * CALLERPRES() is now deprecated because the name and number have their
> own presentation values.
>
> * Fixed app_alarmreceiver.c write_metadata(). The workstring[] could
> contain garbage. It also can only contain the caller id number so using
> ast_callerid_parse() on it is silly. There was also a typo in the
> CALLERNAME if test.
>
> * Fixed app_rpt.c using ast_callerid_parse() on the channel's caller id
> number string. ast_callerid_parse() alters the given buffer which in this
> case is the channel's caller id number string. Then using
> ast_shrink_phone_number() could alter it even more.
>
> * Fixed caller ID name and number memory leak in chan_usbradio.c.
>
> * Fixed uninitialized char arrays cid_num[] and cid_name[] in
> sig_analog.c.
>
> * Protected access to a caller channel with lock in chan_sip.c.
>
> * Clarified intent of code in app_meetme.c sla_ring_station() and
> dial_trunk(). Also made save all caller ID data instead of just the name
> and number strings.
>
> * Simplified cdr.c set_one_cid(). It hand coded the ast_callerid_merge()
> function.
>
> * Corrected some weirdness with app_privacy.c's use of caller
> presentation.
>
>
> Diffs
> -----
>
> /trunk/UPGRADE.txt 269928
> /trunk/addons/chan_ooh323.c 269928
> /trunk/apps/app_alarmreceiver.c 269928
> /trunk/apps/app_amd.c 269928
> /trunk/apps/app_dial.c 269928
> /trunk/apps/app_directed_pickup.c 269928
> /trunk/apps/app_disa.c 269928
> /trunk/apps/app_dumpchan.c 269928
> /trunk/apps/app_fax.c 269928
> /trunk/apps/app_followme.c 269928
> /trunk/apps/app_macro.c 269928
> /trunk/apps/app_meetme.c 269928
> /trunk/apps/app_minivm.c 269928
> /trunk/apps/app_osplookup.c 269928
> /trunk/apps/app_parkandannounce.c 269928
> /trunk/apps/app_privacy.c 269928
> /trunk/apps/app_queue.c 269928
> /trunk/apps/app_readexten.c 269928
> /trunk/apps/app_rpt.c 269928
> /trunk/apps/app_setcallerid.c 269928
> /trunk/apps/app_sms.c 269928
> /trunk/apps/app_stack.c 269928
> /trunk/apps/app_talkdetect.c 269928
> /trunk/apps/app_voicemail.c 269928
> /trunk/apps/app_while.c 269928
> /trunk/apps/app_zapateller.c 269928
> /trunk/channels/chan_agent.c 269928
> /trunk/channels/chan_console.c 269928
> /trunk/channels/chan_dahdi.c 269928
> /trunk/channels/chan_gtalk.c 269928
> /trunk/channels/chan_h323.c 269928
> /trunk/channels/chan_iax2.c 269928
> /trunk/channels/chan_jingle.c 269928
> /trunk/channels/chan_local.c 269928
> /trunk/channels/chan_mgcp.c 269928
> /trunk/channels/chan_misdn.c 269928
> /trunk/channels/chan_oss.c 269928
> /trunk/channels/chan_phone.c 269928
> /trunk/channels/chan_sip.c 269928
> /trunk/channels/chan_skinny.c 269928
> /trunk/channels/chan_unistim.c 269928
> /trunk/channels/chan_usbradio.c 269928
> /trunk/channels/chan_vpb.cc 269928
> /trunk/channels/sig_analog.h 269928
> /trunk/channels/sig_analog.c 269928
> /trunk/channels/sig_pri.c 269928
> /trunk/channels/sig_ss7.c 269928
> /trunk/funcs/func_blacklist.c 269928
> /trunk/funcs/func_callerid.c 269928
> /trunk/funcs/func_connectedline.c 269928
> /trunk/funcs/func_dialplan.c 269928
> /trunk/funcs/func_redirecting.c 269928
> /trunk/include/asterisk/callerid.h 269928
> /trunk/include/asterisk/channel.h 269928
> /trunk/main/app.c 269928
> /trunk/main/callerid.c 269928
> /trunk/main/ccss.c 269928
> /trunk/main/cdr.c 269928
> /trunk/main/cel.c 269928
> /trunk/main/channel.c 269928
> /trunk/main/cli.c 269928
> /trunk/main/dial.c 269928
> /trunk/main/features.c 269928
> /trunk/main/file.c 269928
> /trunk/main/manager.c 269928
> /trunk/main/pbx.c 269928
> /trunk/res/res_agi.c 269928
> /trunk/res/snmp/agent.c 269928
> /trunk/tests/test_substitution.c 269928
>
> Diff: https://reviewboard.asterisk.org/r/702/diff
>
>
> Testing
> -------
>
> Compile testing. SIP and ISDN Q.SIG calls with CONNECTEDLINE, REDIRECTING, and CALLERID values set and read.
>
>
> Thanks,
>
> rmudgett
>
>
More information about the asterisk-dev
mailing list