On 7 Dec 2021, at 09:13, Daniel-Constantin Mierla miconda@gmail.com wrote:

Hello,

this change has side effects and seems to break existing behaviour, one that I spotted:

Sorry,

I looked around and did not find anything that depended on the return code.

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.

Will check that case, must have missed it.

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.

Early morning code. :-)

Thank you for checking.

/O

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/cbd9fc13ab11270df4b541afd9dc9b51...

Author: Olle E. Johansson oej@edvina.net Committer: Olle E. Johansson oej@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/cbd9fc13ab11270df4b541afd9dc9b51... Patch: https://github.com/kamailio/kamailio/commit/cbd9fc13ab11270df4b541afd9dc9b51...

---

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@lists.kamailio.org https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev

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

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@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/cbd9fc13ab11270df4b541afd9dc9b51...

Author: Olle E. Johansson oej@edvina.net Committer: Olle E. Johansson oej@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/cbd9fc13ab11270df4b541afd9dc9b51... Patch: https://github.com/kamailio/kamailio/commit/cbd9fc13ab11270df4b541afd9dc9b51...

---

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