[asterisk-dev] [Code Review] 3070: bridges: Add two new properties to bridges and bridge snapshots - the name of a creator and the name the creator uses to refer to that bridge

[rmudgett]

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



/branches/12/include/asterisk/bridge.h
<https://reviewboard.asterisk.org/r/3070/#comment19853>

    These strings should also be indicated as immutable like the UUID.



/branches/12/include/asterisk/bridge.h
<https://reviewboard.asterisk.org/r/3070/#comment19850>

    Change:
    Pointer to name for special named bridges
    to:
    Name given to the bridge by its creator
    



/branches/12/main/bridge.c
<https://reviewboard.asterisk.org/r/3070/#comment19852>

    Rather than init the stringfields which can fail for this dummy bridge, just set the string field uniqueid pointer in the struct init.
    
    .uniqueid = bridge->uniqueid,
    
    You should not have to allocate memory to destroy something.
    
    Probably should do the creator and name strings as well for completeness in case the bridge technology destructor wants to reference them for debug messages.



/branches/12/main/bridge.c
<https://reviewboard.asterisk.org/r/3070/#comment19854>

    The creator and name parameters should be added to bridge_base_init() instead.  These parameters are part of the base bridge class initialization and that is also where the uniqueid is generated.



/branches/12/main/bridge.c
<https://reviewboard.asterisk.org/r/3070/#comment19855>

    ast_string_field_set is NULL tollerant so you don't need to use ast_strlen_zero().



/branches/12/main/bridge.c
<https://reviewboard.asterisk.org/r/3070/#comment19856>

    You are leaking memory for every return after this except the last one.  In this case you should be able to do the same thing as with bridge_tech_deferred_destroy().



/branches/12/main/manager_bridges.c
<https://reviewboard.asterisk.org/r/3070/#comment19858>

    Many other places use <unknown> instead.



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3070/#comment19857>

    Stasis could give a name to the bridges it creates.  This could aleviate some creation identification issues being discussed.


- rmudgett


On Dec. 13, 2013, 9:40 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3070/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2013, 9:40 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee, kmoore, Mark Michelson, and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> So there has been at least a little bit of clammoring internally about having the need to be able to tell when a bridge belongs to a specific parking lot or app_confbridge conference. This patch provides two new properties to the bridge.
> 
> BridgeCreator - Provides the name of a system which is responsible for creating the bridge.  This includes things such as 'AgentPool', 'Parking', 'ConfBridge', 'Stasis', and other systems which can create bridges.
> 
> BridgeName - Provides the name given to the bridge to refer to it internally by the creator.
> 
> BridgeCreator may be set or it may be unset (in which case it will appear as a zero length string).  BridgeName will only appear when BridgeCreator is also set, but it is also optional. So you have the following possibilities:
> 
> neither BridgeCreator nor BridgeName
> BridgeCreator but not BridgeName
> BridgeCreator and BridgeName
> 
> 
> Diffs
> -----
> 
>   /branches/12/rest-api/api-docs/bridges.json 403725 
>   /branches/12/res/res_stasis.c 403725 
>   /branches/12/res/parking/parking_bridge.c 403725 
>   /branches/12/res/ari/ari_model_validators.c 403725 
>   /branches/12/res/ari/ari_model_validators.h 403725 
>   /branches/12/main/stasis_bridges.c 403725 
>   /branches/12/main/manager_bridges.c 403725 
>   /branches/12/main/bridge_basic.c 403725 
>   /branches/12/main/bridge.c 403725 
>   /branches/12/include/asterisk/stasis_bridges.h 403725 
>   /branches/12/include/asterisk/bridge_internal.h 403725 
>   /branches/12/include/asterisk/bridge.h 403725 
>   /branches/12/doc/appdocsxml.xslt 403725 
>   /branches/12/apps/app_confbridge.c 403725 
>   /branches/12/apps/app_bridgewait.c 403725 
>   /branches/12/apps/app_agent_pool.c 403725 
> 
> Diff: https://reviewboard.asterisk.org/r/3070/diff/
> 
> 
> Testing
> -------
> 
> Checked output for Manager events displaying Bridge snapshots
> 
> Event: BridgeEnter
> Privilege: call,all
> BridgeUniqueid: a72735f6-04ac-499b-a2da-bb997d6f99ab
> BridgeType: parking
> BridgeTechnology: holding_bridge
> BridgeCreator: Parking
> BridgeName: default
> BridgeNumChannels: 1
> Channel: PJSIP/pjgold-00000000
> ChannelState: 6
> ChannelStateDesc: Up
> CallerIDNum: pjgold
> CallerIDName: pjgold
> ConnectedLineNum: <unknown>
> ConnectedLineName: <unknown>
> AccountCode: 
> Context: default
> Exten: 700
> Priority: 1
> Uniqueid: 1386949677.0
> 
> 
> Checked output of GET /bridges on ARI in petstore:
> 
> [
>   {
>     "id": "a72735f6-04ac-499b-a2da-bb997d6f99ab",
>     "channels": [],
>     "technology": "holding_bridge",
>     "bridge_creator": "Parking",
>     "bridge_class": "parking",
>     "bridge_type": "holding",
>     "bridge_name": "default"
>   },
>   {
>     "id": "80e19b8f-5a04-464a-885c-4c119764bf83",
>     "channels": [
>       "1386949828.4"
>     ],
>     "technology": "holding_bridge",
>     "bridge_creator": "BridgeWait",
>     "bridge_class": "base",
>     "bridge_type": "holding",
>     "bridge_name": "testname"
>   }
> ]
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131214/3b06e2d9/attachment-0001.html>