Re: [PATCH] http admin commands + userguide

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