[asterisk-dev] [Code Review]: Add FEATURE() and FEATUREMAP() functions.

Sat Apr 21 02:39:42 CDT 2012

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> >

Thanks for the review!

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/features.h, lines 63-64
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27307#file27307line63>
> >
> >     You'll see that in main/features.c I suggest an alternate method of customizing features using an ao2_container. Doing it the way I suggest would make all mappable features customizable, thus negating the need for this flag.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 3138-3139
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3138>
> >
> >     I have a suggestion for a different way of implementing this.
> >     
> >     Rather than having individual fields for atxfer, blindxfer, etc, have an ao2_container that contains structures like the following:
> >     
> >     struct channel_feature {
> >         char *name;
> >         char exten[FEATURE_MAX_LEN];
> >     };
> >     
> >     This way, a new buffer doesn't have to be added for each builtin feature that the channel overrides. I have further notes in this file where using this method makes coding a bit simpler. The overall result is smaller code that will automatically handle any builtin feature code.

Very nice idea, done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 3141-3143
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3141>
> >
> >     I understand you're doing this because the bounty specifically mentioned "parkingtime" as the only FEATURE() dialplan function example, but I really feel that more items should be configurable here.
> >     
> >     The ideal way to do this would be to put all the general config items that are currently individual globals into a configuration struct. This datastore would also have a configuration struct. When the datastore is created, the configuration struct would inherit the values in the global configuration struct initially. Then, values could be overridden by the FEATURE() dialplan function. When the time came to get the properly configured value, then getting the value from the datastore would *always* result in the correct value being retrieved.
> >     
> >     I think the "ideal" way is adding too much scope creep for this feature addition though, especially given that Terry is currently working on overhauling configuration and will eventually get here.
> >     
> >     I guess all I'm going to suggest is that a XXX comment be added for now saying to come back and do this the right way once configuration has been updated in features.c.

You're right.  I left a note for now.

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, line 3198
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3198>
> >
> >     Since this can fail to find an extension properly, it should return different values for success or failure.

done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 3219-3231
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3219>
> >
> >     Reverse the if check to check if the feature_ds doesn't exist and bail out early if it doesn't. That way you reduce the indentation of this if block by some.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 3220-3230
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3220>
> >
> >     Using the ao2_container method I suggested earlier, This entire section would be an 
> >     
> >     ao2_find(feature_ds->container, feature_name, OBJ_KEY);
> >     
> >     rather than iterating over all possible feature codes.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 3373-3376
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line3373>
> >
> >     If you take the suggestion of having builtin_feature_get_exten() return whether it succeeded, you can check its return value here instead of seeing if feature_exten is empty. This also means you wouldn't have to initialize feature_exten when you declare it, but that's a minor thing.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 8510-8514
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line8510>
> >
> >     If you take my suggestion of having builtin_feature_get_exten() return a value, then you can just call builtin_feature_get_exten() here without checking what data is first. builtin_feature_get_exten() will check to be sure that the feature being asked for is one it knows about. You can then use the return value of builtin_feature_get_exten() to know if invalid input was given.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, lines 8536-8544
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line8536>
> >
> >     If you use an ao2_container like I was suggesting, then you can avoid this if-else ladder.
> >     
> >     You can use ast_find_call_feature() to determine if 'data' is a valid feature name. Then if it is, you can ao2_alloc() a channel_feature struct that has the feature name (data) and the exten (value). Then just ao2_link() the structure into the datastore's container.

Done

> On April 17, 2012, 3:46 p.m., Mark Michelson wrote:
> > /trunk/main/features.c, line 8546
> > <https://reviewboard.asterisk.org/r/1871/diff/1/?file=27308#file27308line8546>
> >
> >     Nitpick: Since this is reached both for error and non-error conditions, give the label a more generic name like "cleanup" or "end"

Removed the goto entirely

- Russell

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1871/#review6006
-----------------------------------------------------------

On April 15, 2012, 12:37 a.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1871/
> -----------------------------------------------------------
> 
> (Updated April 15, 2012, 12:37 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds two new dialplan functions: FEATURE() and FEATUREMAP().
> FEATURE() is indended to be used to allow customizing general feature
> settings on a per-channel basis.  It currently allows you to set a
> custom parkingtime for a channel.
> 
> FEATUREMAP() is similar, but allows customizing the built-in feature
> mappings on a per-channel basis.  Currently you can customize the digit
> string for activating blind or attended transfers.
> 
> More options and feature mappings could be added to this over time as
> desired.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/features.h 362142 
>   /trunk/main/features.c 362142 
> 
> Diff: https://reviewboard.asterisk.org/r/1871/diff
> 
> 
> Testing
> -------
> 
> Using the following dialplan:
> 
> 1) Dial 101, make sure normal transfers work (I tested transferring to 100).
> 2) Dial 102, make sure transfers using channel specific feature mappings work.
> 3) Dial 101, transfer to parking and observe normal parkingtime.
> 4) Dial 103, transfer to parking and verify that the custom parkingtime is used.
> 
> 
> [featuretest]
> 
> include => parkedcalls
> 
> ; Dummy extension for a Local channel.  We'll call this and transfer it.
> exten => foo,1,Answer()
>     same => n(wait),Wait(300)
>     same => n,Goto(wait)
> 
> ; Transfer target.
> exten => 100,1,Answer()
>     same => n(wait),Wait(300)
>     same => n,Goto(wait)
> 
> ; Call this and make sure blindxfer and atxfer still work using default mappings.
> exten => 101,1,Answer()
>     same => n,Verbose(0,Regular transfer test.)
>     same => n,Verbose(0,Blind Transfer: ${FEATUREMAP(blindxfer)})
>     same => n,Verbose(0,Attended Transfer: ${FEATUREMAP(atxfer)})
>     same => n,Dial(Local/foo at featuretest,,tT)
> 
> ; Call this and make sure blindxfer and atxfer work with custom mappings.
> exten => 102,1,Answer()
>     same => n,Verbose(0,Custom transfer test.)
>     same => n,Verbose(0,Setting blindxfer to '77' and atxfer to '88')
>     same => n,Set(FEATUREMAP(blindxfer)=77)
>     same => n,Set(FEATUREMAP(atxfer)=88)
>     same => n,Verbose(0,Blind Transfer: ${FEATUREMAP(blindxfer)})
>     same => n,Verbose(0,Attended Transfer: ${FEATUREMAP(atxfer)})
>     same => n,Dial(Local/foo at featuretest,,tT)
> 
> ; Test setting a parkingtime.  Start with calling '101' and then parking.
> ; Then, call this and verify that the new parkingtime is used.
> exten => 103,1,Answer()
>     same => n,Verbose(0,Test custom parkingtime.)
>     same => n,Verbose(0,Blind Transfer: ${FEATUREMAP(blindxfer)})
>     same => n,Dial(Local/foo at featuretest,,tTU(setparkingtime^3))
> 
> 
> ; GoSub routine for setting parkingtime on a dialed channel.
> [setparkingtime]
> 
> exten => s,1,Verbose(0,Setting custom parkingtime to '${ARG1}')
>     same => n,Verbose(0,Old value: ${FEATURE(parkingtime)})
>     same => n,Set(FEATURE(parkingtime)=${ARG1})
>     same => n,Verbose(0,New value: ${FEATURE(parkingtime)})
>     same => n,Return()
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120421/0aef8e2b/attachment-0001.htm>