Re: [PATCH] http admin commands + userguide

Hi,

I looked through this patch but found it too compilcated. I just don't  
have time to
write properly review...

sorry...

Thanks,
Alexander Malysh

Am 21.08.2009 um 11:44 schrieb Hillel:

> Hi Alex,
>
> As you picked up some bugs in the last version of this patch, any  
> chance to
> review and commit it or just to give comments on using the patch in a
> production environment.
> If you don't see any problems we are keen to use it in production as  
> it
> improves the use of http admin.  Thanks to Alexander Malysh for  
> submitting
> it.
>
> Thanks
>
> ----------------------------------------------------------------------
>
> Date: Thu, 23 Jul 2009 12:07:48 +0200
> From: Alexander Malysh <[email protected]>
> Subject: Re: [PATCH] http admin commands + userguide
> To: Alejandro Guerrieri <[email protected]>
> Cc: Kannel Devel <[email protected]>
> Message-ID: <[email protected]>
> Content-Type: text/plain; charset="windows-1252"; Format="flowed";
> 	DelSp="yes"
>
> 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>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>