Re: [PATCH] http admin commands + userguide

Hi Alex,

sorry for delay, here some comments:

      gw_rwlock_wrlock(&smsc_list_lock);
+    gwlist_add_producer(smsc_list);

you don't need to add producer to smsc_list because all  
synchronization should go via rwlock smsc_list_lock.
The same seems also the case for smsc2_remove_smsc/smsc2_add_smsc....

+
+    /* reload the groups from the config file */
+    if (bb_reload_smsc_groups() != 0) {
+        gwlist_remove_producer(smsc_list);
+        gw_rwlock_unlock(&smsc_list_lock);
+        return -1;
          }

I think it would be better to try to reload config and only then  
shutdown/start SMSC links. Because you should at least ensure
that if config file is not readable that you have all old SMSC links  
intact.

+            if (conn != NULL) {
+                gwlist_append(smsc_list, conn);
+                smscconn_start(conn);
+                success = 1;
          }

Please fix indentation...

Thanks,
Alex

Am 09.07.2009 um 16:04 schrieb Alejandro Guerrieri:

> Alex,
>
> Please try this one.
>
> It works by first stopping and removing all running instances, then  
> it reloads the groups from the config file and tries to add and  
> start the instances again. It also fixes a bug on remove-smsc (some  
> instances would fail to be removed when sharing the same admin-id).
>
> Things to note:
>
> 1. I've tried the scenario you've depicted yesterday and now it  
> works as expected (the extra instances are removed).
> 2. You don't need to stop the smsc first to re-start it. Since the  
> start process removes the running instances, you can run "start- 
> smsc" and it will stop->remove->reload->add->start. Perhaps "restart- 
> smsc" or "reload-smsc" would be more appropiate, to be honest,  
> though that would probably confuse a few people.
>
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
> <kannel-http-admin-fix.patch>
> On 09/07/2009, at 9:21, Alexander Malysh wrote:
>
>>
>> Am 09.07.2009 um 02:23 schrieb Alejandro Guerrieri:
>>
>>> Alex,
>>>
>>> I'm fixing it. I'll be back with a new patch tomorrow.
>>
>> Thanks!
>>
>>>
>>> Regards,
>>> --
>>> Alejandro Guerrieri
>>> [email protected]
>>>
>>>
>>>
>>> On 08/07/2009, at 22:23, Alexander Malysh wrote:
>>>
>>>> Hi again,
>>>>
>>>> Am 08.07.2009 um 18:28 schrieb Alejandro Guerrieri:
>>>>
>>>>> Alex,
>>>>>
>>>>> I've already fixed the warnings (two variables declared but not  
>>>>> used).
>>>>>
>>>>> Regarding the example you've given, that's why I've noted that  
>>>>> you shouldn't play with the id/admin-id's.
>>>>>
>>>>> Under my understanding, restart is meant to be used when you  
>>>>> need to modify some parameters. Modifying the number of binds  
>>>>> qualifies for a remove-smsc/add-smsc.
>>>>>
>>>>> On the scenario you depict, if you remove-smsc A and then add- 
>>>>> smsc A, it'll do as expected.
>>>>>
>>>>> A possible approach would be to replace restart-smsc for:
>>>>>
>>>>> get lock
>>>>> remove-smsc
>>>>> add-smsc
>>>>> release lock
>>>>>
>>>>> Or just leave it as it is and document it better?
>>>>
>>>> this is bug and bugs should not be documented their should be  
>>>> fixed ;)
>>>> Before your patch was applied it was not possible to provoke such  
>>>> situation now it's possible and
>>>> easy fixable. I would like to see patch to fix it or I will do  
>>>> this myself :)
>>>>
>>>> Should I fix it or you?
>>>>
>>>> I just already see user complains :)
>>>>
>>>>>
>>>>> What do you think?
>>>>> --
>>>>> Alejandro Guerrieri
>>>>> [email protected]
>>>>>
>>>>>
>>>>>
>>>>> On 08/07/2009, at 17:40, Alexander Malysh wrote:
>>>>>
>>>>>> Hi Alex,
>>>>>>
>>>>>> sorry I have not seen this before:
>>>>>> gw/bb_smscconn.c: In function ‘smsc2_remove_smsc’:
>>>>>> gw/bb_smscconn.c:839: warning: unused variable ‘smscid’
>>>>>> gw/bb_smscconn.c: In function ‘smsc2_add_smsc’:
>>>>>> gw/bb_smscconn.c:870: warning: unused variable ‘smsc_type’
>>>>>>
>>>>>> Please fix these warnings. Could you please test compile at  
>>>>>> your devel host with
>>>>>> 	./configure --enable-warnings ...
>>>>>> then you will see these :)
>>>>>>
>>>>>> And I think that now, with config reload, smsc2_restart_smsc  
>>>>>> function don't work as expected.
>>>>>> There is example:
>>>>>>
>>>>>> group = smsc
>>>>>> smsc-id = A
>>>>>>
>>>>>> group = smsc
>>>>>> smsc-id = A
>>>>>>
>>>>>> group = smsc
>>>>>> smsc-id = A
>>>>>>
>>>>>> -> start bearerbox
>>>>>>
>>>>>> -> reconfigure
>>>>>>
>>>>>> group = smsc
>>>>>> smsc-id = A
>>>>>>
>>>>>> -> restart_smsc(A)
>>>>>>
>>>>>> first found smsc-id = A will be restarted but the second will  
>>>>>> not found in the config
>>>>>> and error will be logged, leaving the second and third  
>>>>>> instances. This is not a expected behavior.
>>>>>> I would expect that all 3 instances will be shutdown and only  
>>>>>> one will be running after restarting.
>>>>>>
>>>>>> I propose to change it in following sequence:
>>>>>> 	- get lock
>>>>>> 	- shutdown all instances with this id
>>>>>> 	- start all found instances from new config
>>>>>> 	- release lock
>>>>>>
>>>>>> Thanks,
>>>>>> Alex
>>>>>>
>>>>>> Am 08.07.2009 um 16:30 schrieb Alejandro Guerrieri:
>>>>>>
>>>>>>> Commited to CVS.
>>>>>>>
>>>>>>> Regards,
>>>>>>> --
>>>>>>> Alejandro Guerrieri
>>>>>>> [email protected]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 08/07/2009, at 15:12, Alexander Malysh wrote:
>>>>>>>
>>>>>>>> no objections from me...
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alex
>>>>>>>>
>>>>>>>> Am 08.07.2009 um 14:52 schrieb Alejandro Guerrieri:
>>>>>>>>
>>>>>>>>> Here's the patch with the userguide part.
>>>>>>>>>
>>>>>>>>> If no objections, I'll commit it later.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> --
>>>>>>>>> Alejandro Guerrieri
>>>>>>>>> [email protected]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> <kannel-http-admin-ug.patch>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>