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

[opticron]

> On March 7, 2013, 1:18 p.m., David Lee wrote:
> > team/dlee/stasis-core/main/stasis_cache.c, line 222
> > <https://reviewboard.asterisk.org/r/2367/diff/3/?file=33858#file33858line222>
> >
> >     You could just || this with the prior clause.

Done.


> On March 7, 2013, 1:18 p.m., David Lee wrote:
> > team/dlee/stasis-core/include/asterisk/stasis.h, line 219
> > <https://reviewboard.asterisk.org/r/2367/diff/3/?file=33857#file33857line219>
> >
> >     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.

It only matters for messages that are distributed which are/will be MWI and device state changes.  I'll move it into the message data.


> On March 7, 2013, 1:18 p.m., David Lee wrote:
> > team/dlee/stasis-core/main/stasis_message.c, line 146
> > <https://reviewboard.asterisk.org/r/2367/diff/3/?file=33859#file33859line146>
> >
> >     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.

It should be safe since it's a struct copy and the only member is an embedded array, but I'll change it to a memory copy when I move it into the message data.


- opticron


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


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/c4ac52b3/attachment.htm>