Re: [PATCH] http admin commands + userguide

Hi Alex,

sorry, too busy to review...

Thanks,
Alex

Am 23.07.2009 um 11:54 schrieb Alejandro Guerrieri:

> Comments?
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On 16/07/2009, at 18:04, Alejandro Guerrieri wrote:
>
>> Ok, here's a new version with the producer lock removed and a new  
>> approach to restart the binds. I had to rewrite a good part of it  
>> to make it "transactional" and be able to roll-back to the original  
>> group list if the new configuration failed at any point.
>>
>> BTW, I'm not sure about "gwlist_destroy(smsc_groups_copy, NULL);"  
>> shall I destroy the group items?
>>
>> Please review it and let me know what you think.
>>
>> Regards,
>> --
>> Alejandro Guerrieri
>> [email protected]
>>
>> <kannel-http-admin-fix2.patch>
>>
>> On 13/07/2009, at 21:21, Alexander Malysh wrote:
>>
>>> 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>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>