[asterisk-dev] [Code Review] 2492: Initial support for endpoints.

[David Lee]

> On May 6, 2013, 8:48 a.m., Matt Jordan wrote:
> > /trunk/include/asterisk/endpoints.h, lines 88-94
> > <https://reviewboard.asterisk.org/r/2492/diff/1/?file=37107#file37107line88>
> >
> >     This is a little interesting. A SIP peer can have an endpoint related to it, and can add channels to it - which is all fine and good. Doing so, however, doesn't check to see if the endpoint is already shutdown.
> >     
> >     Once an endpoint is shutdown, the message router is destroyed and no further publications can occur. While I'm not sure adding channels to an endpoint after shutdown will cause significant problems, it does mean that consumers of the endpoint will think notifications are going out when they aren't.
> >     
> >     Why would we want to have endpoint shutdown be separate from endpoint destruction?

The explicit shutdown is because of a cyclic reference between the subscription and the endpoint.

This has come up a couple of times, so I decided to write it up on the wiki. Comments welcome - https://wiki.asterisk.org/wiki/x/K4BqAQ


> On May 6, 2013, 8:48 a.m., Matt Jordan wrote:
> > /trunk/main/stasis_endpoints.c, line 123
> > <https://reviewboard.asterisk.org/r/2492/diff/1/?file=37116#file37116line123>
> >
> >     I don't care for variables that are a single character that are not used as a counter in a loop - and it doesn't appear like 'v' is really needed either, as the return value of ast_json_object_set/ast_json_array_append could be checked directly.

I agree that v was a poor choice in naming, but I dislike both multi-line if clauses and lines extending too far past 80 columns. Introducing the extra variable alleviates both.


> On May 6, 2013, 8:48 a.m., Matt Jordan wrote:
> > /trunk/res/res_stasis_http_endpoints.c, lines 99-103
> > <https://reviewboard.asterisk.org/r/2492/diff/1/?file=37117#file37117line99>
> >
> >     This whole segment just looks odd.

This is generated code. The oddness in appearance keeps the generator from getting too complicated.

I wonder if the generated files should be marked as 'application/octet-stream' like the configure script, so they don't show up in code reviews.


- David


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


On May 2, 2013, 2:05 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2492/
> -----------------------------------------------------------
> 
> (Updated May 2, 2013, 2:05 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21421
>     https://issues.asterisk.org/jira/browse/ASTERISK-21421
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> An endpoint is an external device/system that may offer/accept
> channels to/from Asterisk. While this is a very useful concept for end
> users, it is surprisingly not a core concept within Asterisk itself.
> 
> This patch defines ast_endpoint as a separate object, which channel
> drivers may use to expose their concept of an endpoint. As the channel
> driver creates channels, it can use ast_endpoint_add_channel() to
> associate channels to the endpoint. This updated the endpoint
> appropriately, and forwards all of the channel's events to the
> endpoint's topic.
> 
> In order to avoid excessive locking on the endpoint object itself, the
> mutable state is not accessible via getters. Instead, you can create a
> snapshot using ast_endpoint_snapshot_create() to get a consistent
> snapshot of the internal state.
> 
> This patch also includes a set of topics and messages associated with
> endpoints, and implementations of the endpoint-related RESTful
> API. chan_sip was updated to create endpoints with SIP peers, but the
> state of the endpoints is not updated with the state of the peer.
> 
> Along for the ride in this patch is a Stasis test API. This is a
> stasis_message_sink object, which can be subscribed to a Stasis
> topic. It has functions for blocking while waiting for conditions in
> the message sink to be fulfilled.
> 
> (closes issue ASTERISK-21421)
> Review: https://reviewboard.asterisk.org/r/2492/
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 387290 
>   /trunk/channels/sip/include/sip.h 387290 
>   /trunk/include/asterisk/astobj2.h 387290 
>   /trunk/include/asterisk/endpoints.h PRE-CREATION 
>   /trunk/include/asterisk/stasis.h 387290 
>   /trunk/include/asterisk/stasis_endpoints.h PRE-CREATION 
>   /trunk/include/asterisk/stasis_test.h PRE-CREATION 
>   /trunk/main/asterisk.c 387290 
>   /trunk/main/astobj2.c 387290 
>   /trunk/main/channel_internal_api.c 387290 
>   /trunk/main/endpoints.c PRE-CREATION 
>   /trunk/main/stasis_cache.c 387290 
>   /trunk/main/stasis_endpoints.c PRE-CREATION 
>   /trunk/res/res_stasis_http_endpoints.c 387290 
>   /trunk/res/res_stasis_test.c PRE-CREATION 
>   /trunk/res/res_stasis_test.exports.in PRE-CREATION 
>   /trunk/res/stasis_http/resource_endpoints.h 387290 
>   /trunk/res/stasis_http/resource_endpoints.c 387290 
>   /trunk/rest-api/api-docs/endpoints.json 387290 
>   /trunk/tests/test_endpoints.c PRE-CREATION 
>   /trunk/tests/test_stasis_endpoints.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2492/diff/
> 
> 
> Testing
> -------
> 
> Unit tests!
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130506/871ccdde/attachment.htm>