Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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

(Updated Aug. 11, 2014, 7:22 p.m.)


Status
--

This change has been discarded.


Review request for Asterisk Developers.


Bugs: ASTERISK-24068
https://issues.asterisk.org/jira/browse/ASTERISK-24068


Repository: Asterisk


Description
---

This change moves main/manager_*.c to loadable modules, allowing those events 
to be disabled by not loading the modules.  This can be accomplished by 
eventfilter, but eventfilter has a couple issues.  It actually adds more 
overhead to asterisk since the outbound events must be parsed for each AMI 
user.  Additionally it causes skips in SequenceNumber, preventing use of that 
tag to determine if any events were missed during a reconnect.

Besides converting from built-in units to modules, changes are made to VarSet, 
ChannelTalkingStart and ChannelTalkingStop.  They no longer use .to_ami 
callbacks, but instead subscribe to the stasis events like the rest of the 
res_manager_channels events.  A couple functions were also moved from 
manager_bridging.c and manager_channels.c to manager.c since they are still 
needed even if these modules are noload'ed.

AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
committed.  This or r3812 will need to be updated depending on which is 
committed first.


Diffs
-

  /trunk/res/res_manager_system.c PRE-CREATION 
  /trunk/res/res_manager_mwi.c PRE-CREATION 
  /trunk/res/res_manager_endpoints.c PRE-CREATION 
  /trunk/res/res_manager_channels.c PRE-CREATION 
  /trunk/res/res_manager_bridges.c PRE-CREATION 
  /trunk/main/stasis_channels.c 419804 
  /trunk/main/stasis_bridges.c 419804 
  /trunk/main/manager_system.c 419804 
  /trunk/main/manager_mwi.c 419804 
  /trunk/main/manager_endpoints.c 419804 
  /trunk/main/manager_channels.c 419804 
  /trunk/main/manager_bridges.c 419804 
  /trunk/main/manager.c 419804 
  /trunk/main/logger.c 419804 
  /trunk/main/channel.c 419804 
  /trunk/include/asterisk/manager.h 419804 
  /trunk/build_tools/get_documentation 419804 

Diff: https://reviewboard.asterisk.org/r/3811/diff/


Testing
---

Ran some testsuite's to verify some of the events were still being sent to AMI:
tests/manager/originate
tests/apps/channel_redirect
tests/bridge/connected_line_update
tests/feature_call_pickup
tests/apps/dial/dial_answer
tests/apps/chanspy/chanspy_barge
tests/funcs/func_push

This did not provide complete coverage for all effected events, but does verify 
many events from res_manager_channels.c.  Events from other files were not 
tested, though res_manager_channels.c was the most likely to cause problems.


Thanks,

Corey Farrell

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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


My time is 100% consumed by a project for now, I will resubmit when I have time 
to work on this.

- Corey Farrell


On Aug. 1, 2014, 6:22 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated Aug. 1, 2014, 6:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24068
> https://issues.asterisk.org/jira/browse/ASTERISK-24068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_manager_system.c PRE-CREATION 
>   /trunk/res/res_manager_mwi.c PRE-CREATION 
>   /trunk/res/res_manager_endpoints.c PRE-CREATION 
>   /trunk/res/res_manager_channels.c PRE-CREATION 
>   /trunk/res/res_manager_bridges.c PRE-CREATION 
>   /trunk/main/stasis_channels.c 419804 
>   /trunk/main/stasis_bridges.c 419804 
>   /trunk/main/manager_system.c 419804 
>   /trunk/main/manager_mwi.c 419804 
>   /trunk/main/manager_endpoints.c 419804 
>   /trunk/main/manager_channels.c 419804 
>   /trunk/main/manager_bridges.c 419804 
>   /trunk/main/manager.c 419804 
>   /trunk/main/logger.c 419804 
>   /trunk/main/channel.c 419804 
>   /trunk/include/asterisk/manager.h 419804 
>   /trunk/build_tools/get_documentation 419804 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> ---
> 
> Ran some testsuite's to verify some of the events were still being sent to 
> AMI:
> tests/manager/originate
> tests/apps/channel_redirect
> tests/bridge/connected_line_update
> tests/feature_call_pickup
> tests/apps/dial/dial_answer
> tests/apps/chanspy/chanspy_barge
> tests/funcs/func_push
> 
> This did not provide complete coverage for all effected events, but does 
> verify many events from res_manager_channels.c.  Events from other files were 
> not tested, though res_manager_channels.c was the most likely to cause 
> problems.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

> On Aug. 1, 2014, 6:29 p.m., Corey Farrell wrote:
> > /trunk/res/res_manager_channels.c, lines 253-261
> > 
> >
> > channel_state_change has a comment stating that Newchannel, Newstate 
> > and Hangup events are mutually exclusive.  I think this may also apply to 
> > Newexten, NewCallerID and NewAccountcode.  If so this should be moved back 
> > to stasis_channels.c as a .to_ami callback.
> 
> Matt Jordan wrote:
> Hm. These are a bit different, in that the events are inferred from the 
> delta between two snapshots. Generally, a format handler on a Stasis message 
> type is a bit more straight forward: there is a one to one relationship 
> between the message type and the format you want to convert to. A .to_ami 
> handler for cached messages may be possible, but I'm not sure how necessary 
> it is since there is a one to many relationship with the cache message 
> changes and the events it creates (and, in fact, there are times when a 
> change in channel state does not generate an AMI event).

A .to_ami can always return NULL.  I realize most stasis message type handlers 
are very simple, but that's not a rule.  From what I can tell the only rule for 
.to_ami is that one stasis message produces zero or one AMI messages.  I bring 
this up since we've decided that .to_ami is the "best" way to go.  At this 
point I have no objection to leaving these as is.


- Corey


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


On Aug. 1, 2014, 6:22 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated Aug. 1, 2014, 6:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24068
> https://issues.asterisk.org/jira/browse/ASTERISK-24068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_manager_system.c PRE-CREATION 
>   /trunk/res/res_manager_mwi.c PRE-CREATION 
>   /trunk/res/res_manager_endpoints.c PRE-CREATION 
>   /trunk/res/res_manager_channels.c PRE-CREATION 
>   /trunk/res/res_manager_bridges.c PRE-CREATION 
>   /trunk/main/stasis_channels.c 419804 
>   /trunk/main/stasis_bridges.c 419804 
>   /trunk/main/manager_system.c 419804 
>   /trunk/main/manager_mwi.c 419804 
>   /trunk/main/manager_endpoints.c 419804 
>   /trunk/main/manager_channels.c 419804 
>   /trunk/main/manager_bridges.c 419804 
>   /trunk/main/manager.c 419804 
>   /trunk/main/logger.c 419804 
>   /trunk/main/channel.c 419804 
>   /trunk/include/asterisk/manager.h 419804 
>   /trunk/build_tools/get_documentation 419804 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> ---
> 
> Ran some testsuite's to verify some of the events were still being sent to 
> AMI:
> tests/manager/originate
> tests/apps/channel_redirect
> tests/bridge/connected_line_update
> tests/feature_call_pickup
> tests/apps/dial/dial_answer
> tests/apps/chanspy/chanspy_barge
> tests/funcs/func_push
> 
> This did not provide complete coverage for all effected events, but does 
> verify many events from res_manager_channels.c.  Events from other files were 
> not tested, though res_manager_channels.c was the most likely to cause 
> problems.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Matt Jordan]

> On Aug. 1, 2014, 5:29 p.m., Corey Farrell wrote:
> > /trunk/main/logger.c, line 839
> > 
> >
> > Is it appropriate / acceptable to document "LogChannel" in two places?  
> > Or do the two documentation sections need to be combined?
> > 
> > OTOH it feels like these events really should have been called 
> > LoggerChannelStart and LoggerChannelStop.

Keep in mind that they are documented in two places because the same event is 
raised under multiple conditions. Note the different synopsis elements:

Raised when a logging channel 
is re-enabled after a reload operation.

Raised when a logging channel 
is disabled.

Additionally, there is an additional field when the LogChannel event is raised 
when being disabled (Reason).

The Python scripts combine these into a single event documentation element. You 
can see this in the Asterisk 11 event documentation:

https://wiki.asterisk.org/wiki/display/AST/Asterisk+11+ManagerEvent_LogChannel

Whether or not these should be renamed into two separate events is a fair 
point. It's certainly the approach we've gone with for the majority of events 
in AMI 2.X, and for any new events that are added. However, splitting these up 
now would be a breaking change to the API. If we're going to go do that, we 
should (a) do it in Asterisk 14, and (b) make sure that we do one comprehensive 
change.


> On Aug. 1, 2014, 5:29 p.m., Corey Farrell wrote:
> > /trunk/res/res_manager_channels.c, lines 253-261
> > 
> >
> > channel_state_change has a comment stating that Newchannel, Newstate 
> > and Hangup events are mutually exclusive.  I think this may also apply to 
> > Newexten, NewCallerID and NewAccountcode.  If so this should be moved back 
> > to stasis_channels.c as a .to_ami callback.

Hm. These are a bit different, in that the events are inferred from the delta 
between two snapshots. Generally, a format handler on a Stasis message type is 
a bit more straight forward: there is a one to one relationship between the 
message type and the format you want to convert to. A .to_ami handler for 
cached messages may be possible, but I'm not sure how necessary it is since 
there is a one to many relationship with the cache message changes and the 
events it creates (and, in fact, there are times when a change in channel state 
does not generate an AMI event).


- Matt


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


On Aug. 1, 2014, 5:22 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated Aug. 1, 2014, 5:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24068
> https://issues.asterisk.org/jira/browse/ASTERISK-24068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_manager_system.c PRE-CREATION 
>   /trunk/res/res_manager_mwi.c PRE-CREATION 
>   /trunk/res/res_manager_endpoints.c PRE-CREATION 
>   /trunk/res/res_manager_channels.c PRE-CREATION 
>   /trunk/res/res_manager_bridges.c PRE-CREATION 
>   /trunk/main/stasis_channels.c 419804 
>   /trunk/main/stasis_bridges.c 419804 
>   /trunk/main/manager_system.c 419804 
>   /trunk/main/manager_mwi.c 419804 
>   /trunk/main/manager_endpoints.c 419804 
>   /trunk/main/manager_channels.c 419804 
>   /trunk/main/manager_bridges.c 419804 
>   /trunk/main/manager.c

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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



/trunk/main/logger.c


Is it appropriate / acceptable to document "LogChannel" in two places?  Or 
do the two documentation sections need to be combined?

OTOH it feels like these events really should have been called 
LoggerChannelStart and LoggerChannelStop.



/trunk/res/res_manager_bridges.c


These events are mutually exclusive, so this handler should be moved back 
to stasis_bridges.c once converted to a .to_ami callback.



/trunk/res/res_manager_channels.c


channel_state_change has a comment stating that Newchannel, Newstate and 
Hangup events are mutually exclusive.  I think this may also apply to Newexten, 
NewCallerID and NewAccountcode.  If so this should be moved back to 
stasis_channels.c as a .to_ami callback.


- Corey Farrell


On Aug. 1, 2014, 6:22 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated Aug. 1, 2014, 6:22 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24068
> https://issues.asterisk.org/jira/browse/ASTERISK-24068
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/res/res_manager_system.c PRE-CREATION 
>   /trunk/res/res_manager_mwi.c PRE-CREATION 
>   /trunk/res/res_manager_endpoints.c PRE-CREATION 
>   /trunk/res/res_manager_channels.c PRE-CREATION 
>   /trunk/res/res_manager_bridges.c PRE-CREATION 
>   /trunk/main/stasis_channels.c 419804 
>   /trunk/main/stasis_bridges.c 419804 
>   /trunk/main/manager_system.c 419804 
>   /trunk/main/manager_mwi.c 419804 
>   /trunk/main/manager_endpoints.c 419804 
>   /trunk/main/manager_channels.c 419804 
>   /trunk/main/manager_bridges.c 419804 
>   /trunk/main/manager.c 419804 
>   /trunk/main/logger.c 419804 
>   /trunk/main/channel.c 419804 
>   /trunk/include/asterisk/manager.h 419804 
>   /trunk/build_tools/get_documentation 419804 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> ---
> 
> Ran some testsuite's to verify some of the events were still being sent to 
> AMI:
> tests/manager/originate
> tests/apps/channel_redirect
> tests/bridge/connected_line_update
> tests/feature_call_pickup
> tests/apps/dial/dial_answer
> tests/apps/chanspy/chanspy_barge
> tests/funcs/func_push
> 
> This did not provide complete coverage for all effected events, but does 
> verify many events from res_manager_channels.c.  Events from other files were 
> not tested, though res_manager_channels.c was the most likely to cause 
> problems.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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

(Updated Aug. 1, 2014, 6:22 p.m.)


Review request for Asterisk Developers.


Changes
---

* Fix build_tools/get_documentation so it picks up all DOCUMENTATION blocks 
from each file
* Use .to_ami callbacks for most events
* Reorder stasis_channels.c / stasis_bridges.c so that callbacks are 
immediately preceding the STASIS_MESSAGE_TYPE_DEFN declaration.
* Scatter DOCUMENATION blocks to be next to the code that they are documenting. 
 This will make it easier to see discrepancies between documentation and code.
* Add missing  tags throughout code, as well as "class" attribute 
missing from managerEventInstance.  This was needed to deal with the fact that 
doc/core-en_US.xml now contains documentation that was previously missed.


Bugs: ASTERISK-24068
https://issues.asterisk.org/jira/browse/ASTERISK-24068


Repository: Asterisk


Description
---

This change moves main/manager_*.c to loadable modules, allowing those events 
to be disabled by not loading the modules.  This can be accomplished by 
eventfilter, but eventfilter has a couple issues.  It actually adds more 
overhead to asterisk since the outbound events must be parsed for each AMI 
user.  Additionally it causes skips in SequenceNumber, preventing use of that 
tag to determine if any events were missed during a reconnect.

Besides converting from built-in units to modules, changes are made to VarSet, 
ChannelTalkingStart and ChannelTalkingStop.  They no longer use .to_ami 
callbacks, but instead subscribe to the stasis events like the rest of the 
res_manager_channels events.  A couple functions were also moved from 
manager_bridging.c and manager_channels.c to manager.c since they are still 
needed even if these modules are noload'ed.

AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
committed.  This or r3812 will need to be updated depending on which is 
committed first.


Diffs (updated)
-

  /trunk/res/res_manager_system.c PRE-CREATION 
  /trunk/res/res_manager_mwi.c PRE-CREATION 
  /trunk/res/res_manager_endpoints.c PRE-CREATION 
  /trunk/res/res_manager_channels.c PRE-CREATION 
  /trunk/res/res_manager_bridges.c PRE-CREATION 
  /trunk/main/stasis_channels.c 419804 
  /trunk/main/stasis_bridges.c 419804 
  /trunk/main/manager_system.c 419804 
  /trunk/main/manager_mwi.c 419804 
  /trunk/main/manager_endpoints.c 419804 
  /trunk/main/manager_channels.c 419804 
  /trunk/main/manager_bridges.c 419804 
  /trunk/main/manager.c 419804 
  /trunk/main/logger.c 419804 
  /trunk/main/channel.c 419804 
  /trunk/include/asterisk/manager.h 419804 
  /trunk/build_tools/get_documentation 419804 

Diff: https://reviewboard.asterisk.org/r/3811/diff/


Testing
---

Ran some testsuite's to verify some of the events were still being sent to AMI:
tests/manager/originate
tests/apps/channel_redirect
tests/bridge/connected_line_update
tests/feature_call_pickup
tests/apps/dial/dial_answer
tests/apps/chanspy/chanspy_barge
tests/funcs/func_push

This did not provide complete coverage for all effected events, but does verify 
many events from res_manager_channels.c.  Events from other files were not 
tested, though res_manager_channels.c was the most likely to cause problems.


Thanks,

Corey Farrell

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

> On July 17, 2014, 5:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.
> 
> Mark Michelson wrote:
> I can answer your question from (1). In main/manager_channels.c, there is 
> the following code:
> 
> manager_topic = ast_manager_get_topic();
> if (!manager_topic) {
> return -1;
> }
> /* lines snipped */
> channel_topic = ast_channel_topic_all_cached();
> if (!channel_topic) {
> return -1;
> }
> 
> topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
> if (!topic_forwarder) {
> return -1;
> }
> 
> What's happening here is that messages on any channel topic get forwarded 
> to the manager as a result of the topic forwarder. Since varset messages are 
> published on a specific channel topic, they get forwarded to the manager, 
> which calls the to_ami vtable callback to format the message and then send it 
> out.
> 
> Even without a .to_ami callback, the stasis publication is still 
> forwarded to the manager topic. The manager topic still gets the message, 
> sees that there is no to_ami callback and does nothing with the forwarded 
> message.
> 
> The current use of the topic forwarder in main/manager_channels.c means 
> that the easiest way I know of to do what you're after (with varset or any 
> other channel topic publication) is exactly what you've done in this review: 
> withhold a to_ami callback and create a subscription to the specific event 
> type in a loadable module. While this is easier to implement, it results in 
> some extra cycles wasted on forwarding to the manager topic when it really 
> doesn't need to be done at all.
> 
> The other, more complicated option would be to define the varset message 
> type in a loadable module. You'd need to create a public function in that 
> module that is used to publish the message since core modules would not be 
> able to reliably reference the message type defined in the module. With the 
> OPTIONAL_API, you can make it so that attempting to call the publication 
> function provided by the module when the module is not loaded will result in 
> a no-op. This would make it so the varset message type would only exist if 
> the appropriate module were loaded. Therefore, attempting to publish a varset 
> message wo

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Matt Jordan]

> On July 17, 2014, 4:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.
> 
> Mark Michelson wrote:
> I can answer your question from (1). In main/manager_channels.c, there is 
> the following code:
> 
> manager_topic = ast_manager_get_topic();
> if (!manager_topic) {
> return -1;
> }
> /* lines snipped */
> channel_topic = ast_channel_topic_all_cached();
> if (!channel_topic) {
> return -1;
> }
> 
> topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
> if (!topic_forwarder) {
> return -1;
> }
> 
> What's happening here is that messages on any channel topic get forwarded 
> to the manager as a result of the topic forwarder. Since varset messages are 
> published on a specific channel topic, they get forwarded to the manager, 
> which calls the to_ami vtable callback to format the message and then send it 
> out.
> 
> Even without a .to_ami callback, the stasis publication is still 
> forwarded to the manager topic. The manager topic still gets the message, 
> sees that there is no to_ami callback and does nothing with the forwarded 
> message.
> 
> The current use of the topic forwarder in main/manager_channels.c means 
> that the easiest way I know of to do what you're after (with varset or any 
> other channel topic publication) is exactly what you've done in this review: 
> withhold a to_ami callback and create a subscription to the specific event 
> type in a loadable module. While this is easier to implement, it results in 
> some extra cycles wasted on forwarding to the manager topic when it really 
> doesn't need to be done at all.
> 
> The other, more complicated option would be to define the varset message 
> type in a loadable module. You'd need to create a public function in that 
> module that is used to publish the message since core modules would not be 
> able to reliably reference the message type defined in the module. With the 
> OPTIONAL_API, you can make it so that attempting to call the publication 
> function provided by the module when the module is not loaded will result in 
> a no-op. This would make it so the varset message type would only exist if 
> the appropriate module were loaded. Therefore, attempting to publish a varset 
> message wo

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

> On July 17, 2014, 5:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.
> 
> Mark Michelson wrote:
> I can answer your question from (1). In main/manager_channels.c, there is 
> the following code:
> 
> manager_topic = ast_manager_get_topic();
> if (!manager_topic) {
> return -1;
> }
> /* lines snipped */
> channel_topic = ast_channel_topic_all_cached();
> if (!channel_topic) {
> return -1;
> }
> 
> topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
> if (!topic_forwarder) {
> return -1;
> }
> 
> What's happening here is that messages on any channel topic get forwarded 
> to the manager as a result of the topic forwarder. Since varset messages are 
> published on a specific channel topic, they get forwarded to the manager, 
> which calls the to_ami vtable callback to format the message and then send it 
> out.
> 
> Even without a .to_ami callback, the stasis publication is still 
> forwarded to the manager topic. The manager topic still gets the message, 
> sees that there is no to_ami callback and does nothing with the forwarded 
> message.
> 
> The current use of the topic forwarder in main/manager_channels.c means 
> that the easiest way I know of to do what you're after (with varset or any 
> other channel topic publication) is exactly what you've done in this review: 
> withhold a to_ami callback and create a subscription to the specific event 
> type in a loadable module. While this is easier to implement, it results in 
> some extra cycles wasted on forwarding to the manager topic when it really 
> doesn't need to be done at all.
> 
> The other, more complicated option would be to define the varset message 
> type in a loadable module. You'd need to create a public function in that 
> module that is used to publish the message since core modules would not be 
> able to reliably reference the message type defined in the module. With the 
> OPTIONAL_API, you can make it so that attempting to call the publication 
> function provided by the module when the module is not loaded will result in 
> a no-op. This would make it so the varset message type would only exist if 
> the appropriate module were loaded. Therefore, attempting to publish a varset 
> message wo

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Matt Jordan]

> On July 17, 2014, 4:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.
> 
> Mark Michelson wrote:
> I can answer your question from (1). In main/manager_channels.c, there is 
> the following code:
> 
> manager_topic = ast_manager_get_topic();
> if (!manager_topic) {
> return -1;
> }
> /* lines snipped */
> channel_topic = ast_channel_topic_all_cached();
> if (!channel_topic) {
> return -1;
> }
> 
> topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
> if (!topic_forwarder) {
> return -1;
> }
> 
> What's happening here is that messages on any channel topic get forwarded 
> to the manager as a result of the topic forwarder. Since varset messages are 
> published on a specific channel topic, they get forwarded to the manager, 
> which calls the to_ami vtable callback to format the message and then send it 
> out.
> 
> Even without a .to_ami callback, the stasis publication is still 
> forwarded to the manager topic. The manager topic still gets the message, 
> sees that there is no to_ami callback and does nothing with the forwarded 
> message.
> 
> The current use of the topic forwarder in main/manager_channels.c means 
> that the easiest way I know of to do what you're after (with varset or any 
> other channel topic publication) is exactly what you've done in this review: 
> withhold a to_ami callback and create a subscription to the specific event 
> type in a loadable module. While this is easier to implement, it results in 
> some extra cycles wasted on forwarding to the manager topic when it really 
> doesn't need to be done at all.
> 
> The other, more complicated option would be to define the varset message 
> type in a loadable module. You'd need to create a public function in that 
> module that is used to publish the message since core modules would not be 
> able to reliably reference the message type defined in the module. With the 
> OPTIONAL_API, you can make it so that attempting to call the publication 
> function provided by the module when the module is not loaded will result in 
> a no-op. This would make it so the varset message type would only exist if 
> the appropriate module were loaded. Therefore, attempting to publish a varset 
> message wo

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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

(Updated July 21, 2014, 4:56 a.m.)


Review request for Asterisk Developers.


Changes
---

Add JIRA ticket.


Bugs: ASTERISK-24068
https://issues.asterisk.org/jira/browse/ASTERISK-24068


Repository: Asterisk


Description
---

This change moves main/manager_*.c to loadable modules, allowing those events 
to be disabled by not loading the modules.  This can be accomplished by 
eventfilter, but eventfilter has a couple issues.  It actually adds more 
overhead to asterisk since the outbound events must be parsed for each AMI 
user.  Additionally it causes skips in SequenceNumber, preventing use of that 
tag to determine if any events were missed during a reconnect.

Besides converting from built-in units to modules, changes are made to VarSet, 
ChannelTalkingStart and ChannelTalkingStop.  They no longer use .to_ami 
callbacks, but instead subscribe to the stasis events like the rest of the 
res_manager_channels events.  A couple functions were also moved from 
manager_bridging.c and manager_channels.c to manager.c since they are still 
needed even if these modules are noload'ed.

AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
committed.  This or r3812 will need to be updated depending on which is 
committed first.


Diffs
-

  /trunk/main/stasis_channels.c 418738 
  /trunk/main/manager_system.c HEAD 
  /trunk/main/manager_system.c 418738 
  /trunk/main/manager_mwi.c HEAD 
  /trunk/main/manager_mwi.c 418738 
  /trunk/main/manager_endpoints.c HEAD 
  /trunk/main/manager_endpoints.c 418738 
  /trunk/main/manager_channels.c HEAD 
  /trunk/main/manager_channels.c 418738 
  /trunk/main/manager_bridges.c HEAD 
  /trunk/main/manager_bridges.c 418738 
  /trunk/main/manager.c 418738 
  /trunk/include/asterisk/manager.h 418738 

Diff: https://reviewboard.asterisk.org/r/3811/diff/


Testing
---

Ran some testsuite's to verify some of the events were still being sent to AMI:
tests/manager/originate
tests/apps/channel_redirect
tests/bridge/connected_line_update
tests/feature_call_pickup
tests/apps/dial/dial_answer
tests/apps/chanspy/chanspy_barge
tests/funcs/func_push

This did not provide complete coverage for all effected events, but does verify 
many events from res_manager_channels.c.  Events from other files were not 
tested, though res_manager_channels.c was the most likely to cause problems.


Thanks,

Corey Farrell

-- 
_
-- 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

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

> On July 17, 2014, 5:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.
> 
> Mark Michelson wrote:
> I can answer your question from (1). In main/manager_channels.c, there is 
> the following code:
> 
> manager_topic = ast_manager_get_topic();
> if (!manager_topic) {
> return -1;
> }
> /* lines snipped */
> channel_topic = ast_channel_topic_all_cached();
> if (!channel_topic) {
> return -1;
> }
> 
> topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
> if (!topic_forwarder) {
> return -1;
> }
> 
> What's happening here is that messages on any channel topic get forwarded 
> to the manager as a result of the topic forwarder. Since varset messages are 
> published on a specific channel topic, they get forwarded to the manager, 
> which calls the to_ami vtable callback to format the message and then send it 
> out.
> 
> Even without a .to_ami callback, the stasis publication is still 
> forwarded to the manager topic. The manager topic still gets the message, 
> sees that there is no to_ami callback and does nothing with the forwarded 
> message.
> 
> The current use of the topic forwarder in main/manager_channels.c means 
> that the easiest way I know of to do what you're after (with varset or any 
> other channel topic publication) is exactly what you've done in this review: 
> withhold a to_ami callback and create a subscription to the specific event 
> type in a loadable module. While this is easier to implement, it results in 
> some extra cycles wasted on forwarding to the manager topic when it really 
> doesn't need to be done at all.
> 
> The other, more complicated option would be to define the varset message 
> type in a loadable module. You'd need to create a public function in that 
> module that is used to publish the message since core modules would not be 
> able to reliably reference the message type defined in the module. With the 
> OPTIONAL_API, you can make it so that attempting to call the publication 
> function provided by the module when the module is not loaded will result in 
> a no-op. This would make it so the varset message type would only exist if 
> the appropriate module were loaded. Therefore, attempting to publish a varset 
> message wo

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Mark Michelson]

> On July 17, 2014, 9:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.
> 
> Corey Farrell wrote:
> (1) I couldn't determine what causes the current code (stasis_channels.c 
> / manager_channels.c) to have manager subscribe these events (or not).  Maybe 
> I was wrong to assume the existence of .to_ami causes the messages to be 
> broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to 
> be subscribed by the manager topic?
> 
> (2) As for reducing boilerplate code, I don't understand how this is the 
> case - the new code for these events are almost the same as the old code.  
> Yes 
> 
> (3) The primary goal of this change is to allow res_manager_channels to 
> be excluded from a build, and replaced with something that produces selected 
> events with a different/reduced format, or use other custom filters.  I view 
> AMI on two levels - a transport protocol (name value pairs resembling HTTP 
> headers), and an application protocol (the default events produced by 
> Asterisk).  Removal of .to_ami isolates the application layer to modules so 
> it can be replaced.  For example ast_manager_build_channel_state_string 
> provides 1 field that is useful to me - UniqueID.  All other fields are clock 
> cycles wasted during production, transmit and consumption.
> 
> (4) After this change .to_ami would be dead-code at best when 
> res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
> might prevent me from producing custom events.

I can answer your question from (1). In main/manager_channels.c, there is the 
following code:

manager_topic = ast_manager_get_topic();
if (!manager_topic) {
return -1;
}
/* lines snipped */
channel_topic = ast_channel_topic_all_cached();
if (!channel_topic) {
return -1;
}

topic_forwarder = stasis_forward_all(channel_topic, manager_topic);
if (!topic_forwarder) {
return -1;
}

What's happening here is that messages on any channel topic get forwarded to 
the manager as a result of the topic forwarder. Since varset messages are 
published on a specific channel topic, they get forwarded to the manager, which 
calls the to_ami vtable callback to format the message and then send it out.

Even without a .to_ami callback, the stasis publication is still forwarded to 
the manager topic. The manager topic still gets the message, sees that there is 
no to_ami callback and does nothing with the forwarded message.

The current use of the topic forwarder in main/manager_channels.c means that 
the easiest way I know of to do what you're after (with varset or any other 
channel topic publication) is exactly what you've done in this review: withhold 
a to_ami callback and create a subscription to the specific event type in a 
loadable module. While this is easier to implement, it results in some extra 
cycles wasted on forwarding to the manager topic when it really doesn't need to 
be done at all.

The other, more complicated option would be to define the varset message type 
in a loadable module. You'd need to create a public function in that module 
that is used to publish the message since core modules would not be able to 
reliably reference the message type defined in the module. With the 
OPTIONAL_API, you can make it so that attempting to call the publication 
function provided by the module when the module is not loaded will result in a 
no-op. This would make it so the varset message type would only exist if the 
appropriate module were loaded. Therefore, attempting to publish a varset 
message would be a no-op if the module were not loaded. It also means, though, 
that if you are attempting to create your own varset AMI message, you are on 
the hook for defining the stasis message type, using the OPTIONAL_AP

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

> On July 17, 2014, 5:41 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_channels.c, lines 1071-1077
> > 
> >
> > I'm not sure why these changes (removal of the .to_ami callback) were 
> > necessary.
> > 
> > Generally, I prefer the .to_ami callbacks to explicit subscription to 
> > message types and construction of messages in the various manager_* modules:
> > 
> > (1) Obtaining the messages in the appropriate modules is done by simply 
> > forwarding the topics to the manager topic. That substantially reduces the 
> > boilerplate code required.
> > 
> > (2) Co-locating the generation of formatting of messages makes it very 
> > easy to update all consumers of a message when a new field is added, 
> > helping keep the code/events similar for all consumers of that message.
> > 
> > Generally, I would much prefer these to be kept, and to have the other 
> > channel related message have .to_ami callbacks implemented. If anything, 
> > the res_manager_channels module should be very small: it should set up a 
> > forwarding relationship between the channel topics and the manager topic 
> > and be done.

(1) I couldn't determine what causes the current code (stasis_channels.c / 
manager_channels.c) to have manager subscribe these events (or not).  Maybe I 
was wrong to assume the existence of .to_ami causes the messages to be 
broadcast to AMI?  If I am wrong then what causes ast_channel_varset_type to be 
subscribed by the manager topic?

(2) As for reducing boilerplate code, I don't understand how this is the case - 
the new code for these events are almost the same as the old code.  Yes 

(3) The primary goal of this change is to allow res_manager_channels to be 
excluded from a build, and replaced with something that produces selected 
events with a different/reduced format, or use other custom filters.  I view 
AMI on two levels - a transport protocol (name value pairs resembling HTTP 
headers), and an application protocol (the default events produced by 
Asterisk).  Removal of .to_ami isolates the application layer to modules so it 
can be replaced.  For example ast_manager_build_channel_state_string provides 1 
field that is useful to me - UniqueID.  All other fields are clock cycles 
wasted during production, transmit and consumption.

(4) After this change .to_ami would be dead-code at best when 
res_manager_channels.so is not loaded.  At worst I'm concerned that .to_ami 
might prevent me from producing custom events.


- Corey


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


On July 16, 2014, 9:14 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated July 16, 2014, 9:14 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/main/stasis_channels.c 418738 
>   /trunk/main/manager_system.c HEAD 
>   /trunk/main/manager_system.c 418738 
>   /trunk/main/manager_mwi.c HEAD 
>   /trunk/main/manager_mwi.c 418738 
>   /trunk/main/manager_endpoints.c HEAD 
>   /trunk/main/manager_endpoints.c 418738 
>   /trunk/main/manager_channels.c HEAD 
>   /trunk/main/manager_channels.c 418738 
>   /trunk/main/manager_bridges.c HEAD 
>   /trunk/main/manager_bridges.c 418738 
>   /trunk/main/manager.c 418738 
>   /trunk/include/asterisk/manager.h 418738 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> ---
> 
> Ran some testsuite's to verify so

Re: [asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Matt Jordan]

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



/trunk/main/stasis_channels.c


I'm not sure why these changes (removal of the .to_ami callback) were 
necessary.

Generally, I prefer the .to_ami callbacks to explicit subscription to 
message types and construction of messages in the various manager_* modules:

(1) Obtaining the messages in the appropriate modules is done by simply 
forwarding the topics to the manager topic. That substantially reduces the 
boilerplate code required.

(2) Co-locating the generation of formatting of messages makes it very easy 
to update all consumers of a message when a new field is added, helping keep 
the code/events similar for all consumers of that message.

Generally, I would much prefer these to be kept, and to have the other 
channel related message have .to_ami callbacks implemented. If anything, the 
res_manager_channels module should be very small: it should set up a forwarding 
relationship between the channel topics and the manager topic and be done.


- Matt Jordan


On July 16, 2014, 8:14 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3811/
> ---
> 
> (Updated July 16, 2014, 8:14 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> ---
> 
> This change moves main/manager_*.c to loadable modules, allowing those events 
> to be disabled by not loading the modules.  This can be accomplished by 
> eventfilter, but eventfilter has a couple issues.  It actually adds more 
> overhead to asterisk since the outbound events must be parsed for each AMI 
> user.  Additionally it causes skips in SequenceNumber, preventing use of that 
> tag to determine if any events were missed during a reconnect.
> 
> Besides converting from built-in units to modules, changes are made to 
> VarSet, ChannelTalkingStart and ChannelTalkingStop.  They no longer use 
> .to_ami callbacks, but instead subscribe to the stasis events like the rest 
> of the res_manager_channels events.  A couple functions were also moved from 
> manager_bridging.c and manager_channels.c to manager.c since they are still 
> needed even if these modules are noload'ed.
> 
> AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
> committed.  This or r3812 will need to be updated depending on which is 
> committed first.
> 
> 
> Diffs
> -
> 
>   /trunk/main/stasis_channels.c 418738 
>   /trunk/main/manager_system.c HEAD 
>   /trunk/main/manager_system.c 418738 
>   /trunk/main/manager_mwi.c HEAD 
>   /trunk/main/manager_mwi.c 418738 
>   /trunk/main/manager_endpoints.c HEAD 
>   /trunk/main/manager_endpoints.c 418738 
>   /trunk/main/manager_channels.c HEAD 
>   /trunk/main/manager_channels.c 418738 
>   /trunk/main/manager_bridges.c HEAD 
>   /trunk/main/manager_bridges.c 418738 
>   /trunk/main/manager.c 418738 
>   /trunk/include/asterisk/manager.h 418738 
> 
> Diff: https://reviewboard.asterisk.org/r/3811/diff/
> 
> 
> Testing
> ---
> 
> Ran some testsuite's to verify some of the events were still being sent to 
> AMI:
> tests/manager/originate
> tests/apps/channel_redirect
> tests/bridge/connected_line_update
> tests/feature_call_pickup
> tests/apps/dial/dial_answer
> tests/apps/chanspy/chanspy_barge
> tests/funcs/func_push
> 
> This did not provide complete coverage for all effected events, but does 
> verify many events from res_manager_channels.c.  Events from other files were 
> not tested, though res_manager_channels.c was the most likely to cause 
> problems.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-- 
_
-- 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

[asterisk-dev] [Code Review] 3811: Move main/manager_*.c to loadable modules

[Corey Farrell]

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

Review request for Asterisk Developers.


Repository: Asterisk


Description
---

This change moves main/manager_*.c to loadable modules, allowing those events 
to be disabled by not loading the modules.  This can be accomplished by 
eventfilter, but eventfilter has a couple issues.  It actually adds more 
overhead to asterisk since the outbound events must be parsed for each AMI 
user.  Additionally it causes skips in SequenceNumber, preventing use of that 
tag to determine if any events were missed during a reconnect.

Besides converting from built-in units to modules, changes are made to VarSet, 
ChannelTalkingStart and ChannelTalkingStop.  They no longer use .to_ami 
callbacks, but instead subscribe to the stasis events like the rest of the 
res_manager_channels events.  A couple functions were also moved from 
manager_bridging.c and manager_channels.c to manager.c since they are still 
needed even if these modules are noload'ed.

AST_MODULE_INFO_STANDARD for all modules will be updated once r3802 is 
committed.  This or r3812 will need to be updated depending on which is 
committed first.


Diffs
-

  /trunk/main/stasis_channels.c 418738 
  /trunk/main/manager_system.c HEAD 
  /trunk/main/manager_system.c 418738 
  /trunk/main/manager_mwi.c HEAD 
  /trunk/main/manager_mwi.c 418738 
  /trunk/main/manager_endpoints.c HEAD 
  /trunk/main/manager_endpoints.c 418738 
  /trunk/main/manager_channels.c HEAD 
  /trunk/main/manager_channels.c 418738 
  /trunk/main/manager_bridges.c HEAD 
  /trunk/main/manager_bridges.c 418738 
  /trunk/main/manager.c 418738 
  /trunk/include/asterisk/manager.h 418738 

Diff: https://reviewboard.asterisk.org/r/3811/diff/


Testing
---

Ran some testsuite's to verify some of the events were still being sent to AMI:
tests/manager/originate
tests/apps/channel_redirect
tests/bridge/connected_line_update
tests/feature_call_pickup
tests/apps/dial/dial_answer
tests/apps/chanspy/chanspy_barge
tests/funcs/func_push

This did not provide complete coverage for all effected events, but does verify 
many events from res_manager_channels.c.  Events from other files were not 
tested, though res_manager_channels.c was the most likely to cause problems.


Thanks,

Corey Farrell

-- 
_
-- 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