[asterisk-dev] [Code Review] ast_callerid restructuring
rmudgett at digium.com
rmudgett at digium.com
Thu Jun 24 15:16:24 CDT 2010
> On 2010-06-23 13:53:52, Mark Michelson wrote:
> > I've now completed the review for all of the caller ID restructuring. I have clicked the "Ship It" box because I haven't found anything that absolutely must be fixed. I do have some suggestions below, but they are not required.
> >
> > While I haven't found any inherent problems in the code, there are still outstanding suggestions from others such as changing the ANI into an ast_party_number, or possibly allowing for multiple party IDs. Keep that in mind.
Changing the ANI to an ast_party_id is in its own subbranch to avoid burying those changes in this review. I am almost ready to post a review request for that change.
> On 2010-06-23 13:53:52, Mark Michelson wrote:
> > /trunk/main/pbx.c, line 4661
> > <https://reviewboard.asterisk.org/r/702/diff/3/?file=10891#file10891line4661>
> >
> > Since you had to change this function call, might as well add a space before the "1" at the end.
Done.
> On 2010-06-23 13:53:52, Mark Michelson wrote:
> > /trunk/funcs/func_callerid.c, lines 298-336
> > <https://reviewboard.asterisk.org/r/702/diff/3/?file=10874#file10874line298>
> >
> > I recommend changing all the inline comments here to doxygen-style comments.
Done. Though I am not sure how helpful these particular doxygen comments will be since they may not get associated properly to the struct member.
> On 2010-06-23 13:53:52, Mark Michelson wrote:
> > /trunk/funcs/func_callerid.c, line 371
> > <https://reviewboard.asterisk.org/r/702/diff/3/?file=10874#file10874line371>
> >
> > This sort of lax string comparison makes me a bit uneasy. Unfortunately, my unease is based on pretty contrived reasons, so I can't really force you to change this. Here are my reasons:
> >
> > 1. This sort of lax comparison allows for "pres" and "presentation" like you want, but it also allows for strings like "presto" and "presbyterian." Not the best reason to want to be more strict with the string comparison, but it does seem weird to allow such input.
> > 2. If we ever expand on these dialplan functions to have new values, and one of those values happens to start with "pres" then extra care must be taken to be sure not to introduce a bug.
> >
> > There are a few other places where strncasecmp is used in this way, and my same comments apply there as well. I think it would be more clear to just explicitly compare the string to "pres" and "presentation" instead of leaving it open like this.
I am just carrying on what was there before. I did eliminate several that did not have any obvious abbreviations like "pres".
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/702/#review2251
-----------------------------------------------------------
On 2010-06-15 10:40:41, rmudgett wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/702/
> -----------------------------------------------------------
>
> (Updated 2010-06-15 10:40:41)
>
>
> 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 270500
> /trunk/addons/chan_ooh323.c 270500
> /trunk/apps/app_alarmreceiver.c 270500
> /trunk/apps/app_amd.c 270500
> /trunk/apps/app_dial.c 270500
> /trunk/apps/app_directed_pickup.c 270500
> /trunk/apps/app_disa.c 270500
> /trunk/apps/app_dumpchan.c 270500
> /trunk/apps/app_fax.c 270500
> /trunk/apps/app_followme.c 270500
> /trunk/apps/app_macro.c 270500
> /trunk/apps/app_meetme.c 270500
> /trunk/apps/app_minivm.c 270500
> /trunk/apps/app_osplookup.c 270500
> /trunk/apps/app_parkandannounce.c 270500
> /trunk/apps/app_privacy.c 270500
> /trunk/apps/app_queue.c 270500
> /trunk/apps/app_readexten.c 270500
> /trunk/apps/app_rpt.c 270500
> /trunk/apps/app_setcallerid.c 270500
> /trunk/apps/app_sms.c 270500
> /trunk/apps/app_stack.c 270500
> /trunk/apps/app_talkdetect.c 270500
> /trunk/apps/app_voicemail.c 270500
> /trunk/apps/app_while.c 270500
> /trunk/apps/app_zapateller.c 270500
> /trunk/channels/chan_agent.c 270500
> /trunk/channels/chan_console.c 270500
> /trunk/channels/chan_dahdi.c 270500
> /trunk/channels/chan_gtalk.c 270500
> /trunk/channels/chan_h323.c 270500
> /trunk/channels/chan_iax2.c 270500
> /trunk/channels/chan_jingle.c 270500
> /trunk/channels/chan_local.c 270500
> /trunk/channels/chan_mgcp.c 270500
> /trunk/channels/chan_misdn.c 270500
> /trunk/channels/chan_oss.c 270500
> /trunk/channels/chan_phone.c 270500
> /trunk/channels/chan_sip.c 270500
> /trunk/channels/chan_skinny.c 270500
> /trunk/channels/chan_unistim.c 270500
> /trunk/channels/chan_usbradio.c 270500
> /trunk/channels/chan_vpb.cc 270500
> /trunk/channels/sig_analog.h 270500
> /trunk/channels/sig_analog.c 270500
> /trunk/channels/sig_pri.c 270500
> /trunk/channels/sig_ss7.c 270500
> /trunk/funcs/func_blacklist.c 270500
> /trunk/funcs/func_callerid.c 270500
> /trunk/funcs/func_connectedline.c 270500
> /trunk/funcs/func_dialplan.c 270500
> /trunk/funcs/func_redirecting.c 270500
> /trunk/include/asterisk/callerid.h 270500
> /trunk/include/asterisk/channel.h 270500
> /trunk/main/app.c 270500
> /trunk/main/callerid.c 270500
> /trunk/main/ccss.c 270500
> /trunk/main/cdr.c 270500
> /trunk/main/cel.c 270500
> /trunk/main/channel.c 270500
> /trunk/main/cli.c 270500
> /trunk/main/dial.c 270500
> /trunk/main/features.c 270500
> /trunk/main/file.c 270500
> /trunk/main/manager.c 270500
> /trunk/main/pbx.c 270500
> /trunk/res/res_agi.c 270500
> /trunk/res/snmp/agent.c 270500
> /trunk/tests/test_substitution.c 270500
>
> 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