[sr-dev] [kamailio/kamailio] rtpengine: Delete flags & delete handling improvement (#1103)

[Richard Fuchs]

rfuchs requested changes on this pull request.

Couple of suggestions on how to improve the flow of logic here. You can leave them unchanged if you prefer, then I will address them myself afterwards.

However, please do document these changes in `doc/rtpengine_admin.xml`, perhaps with an explanation of how they might be useful.

> @@ -1784,6 +1784,8 @@ static int parse_flags(struct ng_flags_parse *ng_flags, struct sip_msg *msg, enu
 			case 6:
 				if (str_eq(&key, "to-tag")) {
 					ng_flags->to = 1;
+					if (val.s && val.len > 0)

If a value is given, you could simply leave `ng_flags->to` alone (as zero) and `goto generic`

> @@ -1967,8 +1970,13 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf, struct sip_
 	if (ng_flags.rtcp_mux && ng_flags.rtcp_mux->child)
 		bencode_dictionary_add(ng_flags.dict, "rtcp-mux", ng_flags.rtcp_mux);
 
-	bencode_dictionary_add_str(ng_flags.dict, "call-id", &callid);
-
+        temp.s = NULL;
+        if (!bencode_dictionary_get_str(ng_flags.dict, "call-id", &temp))

You can get rid of the temp variable and the case distinction below by simply retrieving the given call ID into `callid`. Otherwise, at the very least please rename `temp` to something more meaningful, as there's quite a long distance between this and the place where it's actually being used.

Generally though, instead of doing a `_get_str` here, I would prefer `parse_flags` to take care of this and update a flag in `ng_flags` to signal that a call ID is already present. (Reason is that a dictionary lookup for a constructed dictionary reverts to a linear search, and I cringe at linear searches. :smile: ) The same applies to the fromtag/totag handling below, which can be solved in a similar way.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1103#pullrequestreview-35371108
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20170428/1487028d/attachment-0001.html>