Summary:	ASTERISK-13469: [patch] RemoveQueueMember could remove realtime members too
Reporter:	Fredrik Liljegren (fiddur)	Labels:
Date Opened:	2009-01-28 06:46:19.000-0600	Date Closed:	2009-03-03 15:49:36.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_queue.c.diff ( 1) app_queue.c.diff2 ( 2) app_queue.c.diff3
Description:	If RemoveQueueMember is called for a realtime member, it only returned "Member not dynamic". I added the code to remove realtime members too.  Patch follows.
Comments:	By: Mark Michelson (mmichelson) 2009-01-29 11:58:11.000-0600 The inability to remove realtime queue members from Asterisk is a design decision, not a bug. Realtime configurations are typically treated as "read-only" by Asterisk. Asterisk will read configuration changes and update its internals when something changes externally, but Asterisk typically will not write to a realtime configuration. There are exceptions to this rule, such as the "paused" column for a queue member in queues.conf. Realtime's design goal is to allow for an external configuration source from Asterisk to be read in. Changes to that external configuration source should also be updated via some external means instead of through Asterisk. In my opinion, using Asterisk to update information in realtime defeats the purpose of using realtime configuration in the first place. If you are going to attempt to remove a dynamic realtime queue member, then the logical way of doing so would be to remove the queue member in a similar way to how it was originally added. Asterisk will see this change and also remove the queue member from its local container, too. Now that I have stated my opinion about the philosophical reasons why I think this should not be added, let me state that this patch you have provided would not actually cause any ill effects as far as Asterisk is concerned. As far as your actual patch goes, I think it's fine, except there is a memory leak in remove_realtime_queue_member. You need to call ast_variables_destroy on the pointer returned by ast_load_realtime to free the memory allocated. Even though I have stated that I don't like the addition of this feature on principle, I am not going to close this issue as I would like to get the opinions of other developers to make sure I'm not completely off-base in my reasoning here. By: Fredrik Liljegren (fiddur) 2009-01-30 00:42:36.000-0600 Thank you for pointing out the memory leak.  I see there are more places in app_queue.c that leaks then; I mostly copied update_realtime_member_field and modified - I'm adding ast_variables_destroy to both. Having the standard manager and dialplan applications also affect realtime databases helps to integrate separate applications.  It would extend to more connections between dynamic and realtime, e.g. maybe making dynamic adding of queue-members add to realtime if the queue is specified in realtime and there is a realtime queue member-table.  Then, an application that is only aware of the realtime db can make it's changes there, while you can still use AMI applications (like asternics op_panel) at the same time.  I guess that is a design strategic decision, but I think it would be a useful design, and the right way. On the other hand, I can imagine that people who are used to the realtime tables only changed by their application and not by asterisk wouldn't want this, if they for example cache the tables or don't re-read them assuming noone else changes them. If you decide to go with my way of thinking, I'm prepared to do more coding for that. I will upload a new patch with the memory leaks fixed. By: Digium Subversion (svnbot) 2009-03-03 15:43:34.000-0600 Repository: asterisk Revision: 179971 U   branches/1.6.0/apps/app_queue.c ------------------------------------------------------------------------ r179971 | mmichelson | 2009-03-03 15:43:34 -0600 (Tue, 03 Mar 2009) | 5 lines Fix a memory leak when updating a realtime member field. This was discovered while looking at issue ASTERISK-13469 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=179971 By: Mark Michelson (mmichelson) 2009-03-03 15:49:36.000-0600 Well, I directed other devs to this issue in case they wanted to weigh in with an opinion counter to mine. They obviously don't feel strongly enough to disagree with what either of us have said here, so I'm going to be the bad guy and close this issue out under my earlier stance that realtime configuration changes should happen externally to the Asterisk process unless necessary. If you want to keep these changes you've made in your local copy, that's of course perfectly fine. If you feel that I've made a bad decision here and that you'd like to appeal to more devs directly, feel free to bring this issue up for discussion on the asterisk-dev mailing list.