Re: [PATCH] http admin commands + userguide

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]



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