[asterisk-dev] [Code Review] Add to Stasis message cache dumping and message EIDs

[David Lee]

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



team/dlee/stasis-core/include/asterisk/stasis.h
<https://reviewboard.asterisk.org/r/2367/#comment15366>

    I've got two problems with this.
    
    The first, and easy to fix, is that messages are supposed to be immutable. That's what allows them to be shared across threads without locking. If we think this should be a part of the base message type, I would ditch the setter, add a stasis_message_create_with_eid, in which you specify the EID. The stasis_message_create can still fill it in with the default.
    
    Secondly is whether this is really needed at all. I can only see this being applicable for a small set of the messages that Stasis will deal with; most cases won't care at all. Given that, I would say that if a specific message needs an EID, then it should be a part of that message itself.



team/dlee/stasis-core/main/stasis_cache.c
<https://reviewboard.asterisk.org/r/2367/#comment15365>

    You could just || this with the prior clause.



team/dlee/stasis-core/main/stasis_message.c
<https://reviewboard.asterisk.org/r/2367/#comment15367>

    Don't you need to copy the eid? Otherwise you'll have lifecycle problems knowing when it's safe to dispose of the eid you've given to a message.


- David


On March 7, 2013, 10:50 a.m., opticron wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2367/
> -----------------------------------------------------------
> 
> (Updated March 7, 2013, 10:50 a.m.)
> 
> 
> Review request for Asterisk Developers and David Lee.
> 
> 
> Summary
> -------
> 
> This review covers several items involved in the transition of MWI from the event system to Stasis:
> * Creation of cache dump capability (necessary for pollmailboxes in app_voicemail)
> * Update of the Stasis unit tests to cover those new items
> * Ability to set a server EID on messages
> 
> 
> This addresses bug ASTERISK-21097.
>     https://issues.asterisk.org/jira/browse/ASTERISK-21097
> 
> 
> Diffs
> -----
> 
>   team/dlee/stasis-core/include/asterisk/stasis.h 382509 
>   team/dlee/stasis-core/main/stasis_cache.c 382509 
>   team/dlee/stasis-core/main/stasis_message.c 382509 
>   team/dlee/stasis-core/tests/test_stasis.c 382509 
> 
> Diff: https://reviewboard.asterisk.org/r/2367/diff
> 
> 
> Testing
> -------
> 
> Tested this using the unit tests and in conjunction with the MWI changes they were created for.
> 
> 
> Thanks,
> 
> opticron
> 
>

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