[prev in list] [next in list] [prev in thread] [next in thread]
List: asterisk-dev
Subject: Re: [asterisk-dev] [Code Review] ast_callerid restructuring
From: rmudgett () digium ! com
Date: 2010-06-24 20:16:24
Message-ID: 20100624201624.27981.61205 () hotblack ! digium ! com
[Download RAW message or body]
> 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
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic