@rfuchs commented on this pull request.

An example of the intended usage would be helpful


In src/modules/rtpengine/rtpengine.c:

> +				} else if(str_eq(&key, "all")) {
+					ng_flags->all_legs = 1;
+				} else

I think this is redundant? Just putting the word "all" in the argument string would already lead to "all" being put in the flags list without the need for special handling, no?


In src/modules/rtpengine/rtpengine.c:

> +				else if(str_eq(&key, "from-tags")) {
+					err = "missing value";
+					if(!val.s)
+						goto error;
+
+					if(!ng_flags->from_tags) {
+						ng_flags->from_tags =
+								bencode_list(ng_flags->dict->buffer);
+					}
+					bencode_list_add_str(ng_flags->from_tags, &val);
+				} else

Ok if this is how you want to do it, but some suggestions for alternatives: subscribe request can take either a single from-tag or multiple from-tags as a list, and would treat them the same. So here you could also treat "from-tag" and "from-tags" the same, and making building the list conditional on the opmode.

Or: omit handling altogether and require a modern version of rtpengine which parses the flags internally (parsed_by_module false). In this case you can use syntax like rtpengine_blah("flag flag ... from-tags=[tag1 tag2 tag3] ...")

Up to you, just a hint.


In src/modules/rtpengine/rtpengine.c:

> +	{"rtpengine_subscribe_offer", (cmd_function)rtpengine_subscribe_request_wrap_f, 4,
+	fixup_rtpengine_subscribe_request_v, fixup_free_rtpengine_subscribe_request_v, ANY_ROUTE},
+	{"rtpengine_subscribe_offer", (cmd_function)rtpengine_subscribe_request_wrap_f, 5,
+	fixup_rtpengine_subscribe_request_v, fixup_free_rtpengine_subscribe_request_v, ANY_ROUTE},
+	{"rtpengine_subscribe_answer", (cmd_function)rtpengine_subscribe_answer_wrap_f, 1,
+	fixup_spve_null, fixup_free_spve_null, ANY_ROUTE},
+	{"rtpengine_subscribe_answer", (cmd_function)rtpengine_subscribe_answer_wrap_f, 2,
+	fixup_spve_spve, fixup_free_spve_spve, ANY_ROUTE},
+	{"rtpengine_unsubscribe", (cmd_function)rtpengine_unsubscribe_wrap_f, 1,
+	fixup_spve_null, fixup_free_spve_null, ANY_ROUTE},
+	{"rtpengine_unsubscribe", (cmd_function)rtpengine_unsubscribe_wrap_f, 2,
+	fixup_spve_spve, fixup_free_spve_spve, ANY_ROUTE},

These would have to be added to the documentation


Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.Message ID: <kamailio/kamailio/pull/4274/review/2904627533@github.com>