[asterisk-dev] [Code Review] ast_callerid restructuring
rmudgett at digium.com
rmudgett at digium.com
Tue Jun 15 10:14:09 CDT 2010
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, line 224
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10795#file10795line224>
> >
> > "tolerate"
Done. All changes as a result of this review will be part of the next diff update.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, lines 2587-2596
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10795#file10795line2587>
> >
> > 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().
The primary difference between a copy and a set is intent and as a result how strings are handled. A copy simply copies all source member field values into the destination. A set updates the destination field member values with the source information. For integer members there is no difference. For string members, if the source member is NULL then a set does not update the destination.
Starting with the ast_party_id structure, the set has more differences by adding more update information. The additional update information specifies what subcomponent to set. The additional information is needed if you are setting a party using a structure that was just initialized instead of set initialized. (ast_party_id_init() vs ast_party_id_set_init()) How do you only update the name subcomponent without destroying any values in the number or subaddress subcomponents?
The doxygen comments for the copy and set function uses different words for the parameters. I'm not sure how to document this without the description being longer than the code itself. In fact this description is already larger than the code itself for the copy and set functions.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, lines 1628-1634
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line1628>
> >
> > 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?
Yes you misunderstand. See the previous response.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, line 1686
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line1686>
> >
> > Same comment about the valid flag as with ast_party_name_set_init()
Same response as above.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, line 7485
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line7485>
> >
> > 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.
Done.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, line 7586
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line7586>
> >
> > Remove ast_ from the function name.
Done.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, line 7767
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line7767>
> >
> > Remove ast_ from function name.
Done.
> On 2010-06-14 18:04:48, Mark Michelson wrote:
> > /trunk/main/channel.c, line 7872
> > <https://reviewboard.asterisk.org/r/702/diff/2/?file=10801#file10801line7872>
> >
> > Remove ast_ from function name.
Done.
- rmudgett
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/702/#review2200
-----------------------------------------------------------
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