[sr-dev] git:master:cbd9fc13: htable: Add return code on successful deletion of item, update RPC commands with replies

Tue Dec 7 09:48:54 CET 2021

Hello,

On 07.12.21 09:36, Olle E. Johansson wrote:
> Hi!
>
> Return values seems poorly documented here, so it’s hard to fix stuff.
> ht_api_del_cell in api.c has literally empty docs and the API is not
> covered in the README...

improvements are always welcome :-) ! The module was started in 2008,
probably the focus during that year was not much on documenting internal
c code ...

>
> /**
> *
> */
> int ht_api_del_cell(str *hname, str *name)
> I will reset it to return zero instead of one.

Probably is better to rename the function ht_api_del_cell() to something
else like ht_api_rm_cell(), which returns 1 on removal, then  (re-)add
ht_api_del_cell() as a wrapper around the new function, with code like:

ret = ht_api_rm_cell(...);

return (ret<0)?ret:0;

So the old function (by name) preserves the behaviour and you can use
the new function where you need it.

Cheers,
Daniel

> /O
>
>> On 7 Dec 2021, at 09:13, Daniel-Constantin Mierla <miconda at gmail.com>
>> wrote:
>>
>> Hello,
>>
>> this change has side effects and seems to break existing behaviour, one
>> that I spotted:
>>
>> src/modules/htable/ht_dmq.c
>> 327:            if (ht_dmq_replay_action(action, &htname, &cname, type,
>> &val, mode)!=0) {
>>                         LM_ERR("failed to replay action\n");
>>                         goto error;
>>
>> Inside ht_dmq_replay_action() it is:
>>
>>          return ht_del_cell(ht, cname);
>>
>> Because ht_del_cell() was changed to return 1 in case of successful
>> deletion, practically ends up in error case inside the ht_dmq.c code.
>>
>> There are other places where the del function is used, I haven't checked
>> all of them, but the current form does not seem correct as it is.
>>
>> Cheers,
>> Daniel
>>
>> On 07.12.21 08:53, Olle E. Johansson wrote:
>>> Module: kamailio
>>> Branch: master
>>> Commit: cbd9fc13ab11270df4b541afd9dc9b51517cdd12
>>> URL:
>>> https://github.com/kamailio/kamailio/commit/cbd9fc13ab11270df4b541afd9dc9b51517cdd12
>>>
>>> Author: Olle E. Johansson <oej at edvina.net>
>>> Committer: Olle E. Johansson <oej at edvina.net>
>>> Date: 2021-12-07T08:52:08+01:00
>>>
>>> htable: Add return code on successful deletion of item, update RPC
>>> commands with replies
>>>
>>> ---
>>>
>>> Modified: src/modules/htable/ht_api.c
>>> Modified: src/modules/htable/htable.c
>>>
>>> ---
>>>
>>> Diff:
>>>  https://github.com/kamailio/kamailio/commit/cbd9fc13ab11270df4b541afd9dc9b51517cdd12.diff
>>> Patch:
>>> https://github.com/kamailio/kamailio/commit/cbd9fc13ab11270df4b541afd9dc9b51517cdd12.patch
>>>
>>> ---
>>>
>>> diff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c
>>> index d4310afa3d..45623e0165 100644
>>> --- a/src/modules/htable/ht_api.c
>>> +++ b/src/modules/htable/ht_api.c
>>> @@ -655,6 +655,13 @@ static void ht_cell_unlink(ht_t *ht, int idx,
>>> ht_cell_t *it)
>>> ht->entries[idx].esize--;
>>> }
>>>
>>> +/* Delete htable entry
>>> +
>>> +Return:
>>> +-1 on error in argument
>>> +0 on entry not found
>>> +1 on entry found and deleted
>>> +*/
>>> int ht_del_cell(ht_t *ht, str *name)
>>> {
>>> unsigned int idx;
>>> @@ -689,7 +696,7 @@ int ht_del_cell(ht_t *ht, str *name)
>>> ht_cell_unlink(ht, idx, it);
>>> ht_slot_unlock(ht, idx);
>>> ht_cell_free(it);
>>> -return 0;
>>> +return 1;
>>> }
>>> it = it->next;
>>> }
>>> diff --git a/src/modules/htable/htable.c b/src/modules/htable/htable.c
>>> index a20b3d6e8d..d491e2e767 100644
>>> --- a/src/modules/htable/htable.c
>>> +++ b/src/modules/htable/htable.c
>>> @@ -1438,6 +1438,7 @@ static const char* htable_store_doc[2] = {
>>> static void htable_rpc_delete(rpc_t* rpc, void* c) {
>>> str htname, keyname;
>>> ht_t *ht;
>>> +int res;
>>>
>>> if (rpc->scan(c, "SS", &htname, &keyname) < 2) {
>>> rpc->fault(c, 500, "Not enough parameters (htable name & key name");
>>> @@ -1453,7 +1454,17 @@ static void htable_rpc_delete(rpc_t* rpc,
>>> void* c) {
>>> LM_ERR("dmq replication failed\n");
>>> }
>>>
>>> -ht_del_cell(ht, &keyname);
>>> +res = ht_del_cell(ht, &keyname);
>>> +
>>> +if (res  == -1) {
>>> +rpc->fault(c, 500, "Internal error");
>>> +return;
>>> +} else if (res == 0) {
>>> +rpc->fault(c, 404, "Key not found in htable.");
>>> +return;
>>> +}
>>> +rpc->rpl_printf(c, "Ok. Key deleted.");
>>> +return;
>>> }
>>>
>>> /*! \brief RPC htable.get command to get one item */
>>> @@ -1561,7 +1572,7 @@ static void htable_rpc_sets(rpc_t* rpc, void* c) {
>>> rpc->fault(c, 500, "Failed to set the item");
>>> return;
>>> }
>>> -
>>> +rpc->rpl_printf(c, "Ok. Key set to new value.");
>>> return;
>>> }
>>>
>>> @@ -1596,7 +1607,7 @@ static void htable_rpc_seti(rpc_t* rpc, void* c) {
>>> rpc->fault(c, 500, "Failed to set the item");
>>> return;
>>> }
>>> -
>>> +rpc->rpl_printf(c, "Ok. Key set to new value.");
>>> return;
>>> }
>>>
>>>
>>>
>>> _______________________________________________
>>> Kamailio (SER) - Development Mailing List
>>> sr-dev at lists.kamailio.org
>>> https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
>>
>> -- 
>> Daniel-Constantin Mierla -- www.asipto.com <http://www.asipto.com>
>> www.twitter.com/miconda <http://www.twitter.com/miconda> --
>> www.linkedin.com/in/miconda <http://www.linkedin.com/in/miconda>
>> Kamailio Advanced Training - Online
>>  Feb 21-24, 2022 (America Timezone)
>>  * https://www.asipto.com/sw/kamailio-advanced-training-online/
>>
>
-- 
Daniel-Constantin Mierla -- www.asipto.com
www.twitter.com/miconda -- www.linkedin.com/in/miconda
Kamailio Advanced Training - Online
  Feb 21-24, 2022 (America Timezone)
  * https://www.asipto.com/sw/kamailio-advanced-training-online/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20211207/12d221d4/attachment-0001.htm>