----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3494/#review11925 -----------------------------------------------------------
/branches/12/include/asterisk/stasis_app.h <https://reviewboard.asterisk.org/r/3494/#comment21818> I'm not sure about this callback existing. In general, these callbacks simply jump back out to the cache and get the latest snapshot. There's two problems with this: (1) The callback implies that we convert the event source to a snapshot, but we aren't really converting the current state, we're converting the last known state. If anything, this feels more like a 'get_cached_snapshot' (2) This callback is really only useful in a single place - generation of the user event. Not all event sources are supported either, which means if I'm adding an event source, I may feel like I need to implement this callback - but in reality, I probably shouldn't since the user event can't support it in the first place. Given that there's only three event sources we support, this could just be a switch state: switch (source) { case STASIS_UMOS_CHANNEL: snapshot = ast_channel_snapshot_get_latest(id); break; ... } /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21817> Put this in the header, and change the method signatures to pass in the enum value instead of a string. In general, users of the API know the type of the object they are adding to the blob; this minimizes the amount of string comparisons you're going to need to do. /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21813> Since these can be used to index things, I'd explicitly call out that this is '0' /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21812> Add a 'max array value' - STASIS_UMOS_MAX_VALUE - entry here (more on that later) /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21815> Sticking heterogeneous types into a single ao2_container still feels like the wrong approach here, for a few reasons: (1) The types are known before hand. We know that we have channels, bridges, and endpoint. We can use an array here of some other container, which will be much more efficient then constructing a specific hash based on a prefix type. It also would eliminate code that determines the type of snapshot later. (2) ao2 containers are a bad choice here. They are useful when the pattern of usage is to look up an item by key; however, here the pattern of usage is to iterate over all the items. This should use a list structure or a vector instead. Ideally, what we'd have here would be something like this: struct ast_multi_object_blob { struct ast_json *blob; AST_VECTOR(, void *) snapshots[STASIS_UMOS_MAX]; }; That gives you an array of vectors, each of which is typed to the enum you've already defined. Since this is really an array of pointers to arrays of pointers, it's about as fast as you're going to get. It also doesn't require any custom keying, comparisons, or a lot of ao2 lifetime management that is unnecessary. (As an aside: when we wrote the multi-channel blob stuff originally, we didn't have vectors. It'd probably be good to open an issue to go back and vectorize those). /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21811> This should be removed. /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21808> Spaces. This allocation is also extremely small given how much data is usually going to be packed into a UserEvent. As an example, a single channels snapshot gets a (probably too large) string size of 1024 bytes. I'd bump this up some. /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21810> This can just be ast_free /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21809> You should be able to just call ast_free here. /branches/12/main/stasis.c <https://reviewboard.asterisk.org/r/3494/#comment21807> This documentation is incomplete. Right now, the only parameter it will show as being 'possible' will be a single channel and the UserEvent parameter, which isn't really true. There's also no real benefit defining this in-line in the source file. The only benefit for doing that is when you have multiple events of the same type, or when the parameters of the event can be inferred from the manager_event* call, neither of which is true here. I'd define the event at the top, and specify the parameters that can be present specifically. Parameters can be labelled as being optional. If the parameters are too difficult to list (since there can be a substantial number of them now), at a minimum a description of the event should be added and specify how and what parameters are possible. - Matt Jordan On May 12, 2014, 3:45 p.m., Scott Griepentrog wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3494/ > ----------------------------------------------------------- > > (Updated May 12, 2014, 3:45 p.m.) > > > Review request for Asterisk Developers. > > > Bugs: ASTERISK-22697 > https://issues.asterisk.org/jira/browse/ASTERISK-22697 > > > Repository: Asterisk > > > Description > ------- > > This changes the implementation of UserEvent stasis messaging and adds > additional features: > > 1) Existing channel_user_event stasis messaging replaced with new > multi_object_blob in stasis_user.c > 2) Multi object blob provides ability to signal blob + any number of channel, > bridge, or endpoint snapshots > 3) Changed existing Userevent() app to use new multi object blob - but still > publishes to channel > 4) Added ARI implementation of userevent as > /ari/applications/{applicationName}/user/{eventName} > 5) ARI generated userevent messages are published to application, and also to > AMI if channel present > > > Diffs > ----- > > /branches/12/rest-api/api-docs/events.json 413115 > /branches/12/res/stasis/app.c 413115 > /branches/12/res/res_stasis.c 413115 > /branches/12/res/res_ari_events.c 413115 > /branches/12/res/ari/resource_events.c 413115 > /branches/12/res/ari/resource_events.h 413115 > /branches/12/res/ari/ari_model_validators.c 413115 > /branches/12/res/ari/ari_model_validators.h 413115 > /branches/12/main/stasis_endpoints.c 413115 > /branches/12/main/stasis_channels.c 413115 > /branches/12/main/stasis.c 413115 > /branches/12/main/manager_channels.c 413115 > /branches/12/include/asterisk/stasis_channels.h 413115 > /branches/12/include/asterisk/stasis_app.h 413115 > /branches/12/include/asterisk/stasis.h 413115 > /branches/12/apps/app_userevent.c 413115 > > Diff: https://reviewboard.asterisk.org/r/3494/diff/ > > > Testing > ------- > > Tested with valgrind to insure no invalid references. Some leaks found which > have been attributed to other code to be fixed separately. > > > Thanks, > > Scott Griepentrog > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
