@rfuchs commented on this pull request.

Overall LGTM with some exceptions.

As for style - I would prefer to retain the "subscribe" schema of naming the functions instead of "copy" - took me a while to understand that this refers to making a copy of the media stream.

Also I think it may be worth refactoring rtpp_function_call and split it up into two parts, one of them taking an existing dict as argument, to avoid having the boilerplate if(!extra_dict) in the code. But that's not important and can be done later on separately.

In src/modules/rtpengine/rtpengine.c:

> +	if(sess->callid)
+		bencode_dictionary_add_str(dict, "call-id", sess->callid);
+	else if(sess->msg)
+		bencode_dictionary_add_str(dict, "call-id", &sess->msg->callid->body);
+	if(sess->branch != RTPENGINE_ALL_BRANCHES)
+		bencode_dictionary_add_str(dict, "via-branch", viabranch);
+	if(to_tag && to_tag->len)
+		bencode_dictionary_add_str(dict, "to-tag", to_tag);
+	if(copy_flags & RTP_COPY_MODE_SIPREC) {
+		list = bencode_list(&bencbuf);
+		bencode_dictionary_add(dict, "flags", list);
+		bencode_list_add_string(list, "all");
+		bencode_list_add_string(list, "siprec");
+	} else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) {
+		list = bencode_list(&bencbuf);
+		bencode_list_add_string(list, "all");

The list needs to be added into the dict as flags no?

In src/modules/rtpengine/rtpengine.c:

> +	if(sess->branch != RTPENGINE_ALL_BRANCHES)
+		bencode_dictionary_add_str(dict, "via-branch", viabranch);
+	if(to_tag && to_tag->len)
+		bencode_dictionary_add_str(dict, "to-tag", to_tag);
+	if(copy_flags & RTP_COPY_MODE_SIPREC) {
+		list = bencode_list(&bencbuf);
+		bencode_dictionary_add(dict, "flags", list);
+		bencode_list_add_string(list, "all");
+		bencode_list_add_string(list, "siprec");
+	} else if((copy_flags & RTP_COPY_LEG_BOTH) == RTP_COPY_LEG_BOTH) {
+		list = bencode_list(&bencbuf);
+		bencode_list_add_string(list, "all");
+	} else if(copy_flags & RTP_COPY_LEG_CALLER && sess->from_tag) {
+		bencode_dictionary_add_str(dict, "from-tag", sess->from_tag);
+	} else if(sess->to_tag) {
+		bencode_dictionary_add_str(dict, "from-tag", sess->to_tag);

The to-tag should be to-tag ?

In src/modules/rtpengine/rtpengine.c:

> @@ -3323,9 +3341,18 @@ static bencode_item_t *rtpp_function_call(bencode_buffer_t *bencbuf,
 	}
 
 	/* initialize some basic bencode items */
-	ng_flags.dict = bencode_dictionary(bencbuf);
+	if(!extra_dict) {
+		ng_flags.dict = bencode_dictionary(bencbuf);
+		if(parse_by_module) {
+			ng_flags.flags = bencode_list(bencbuf);
+		}
+	} else {
+		ng_flags.dict = extra_dict;
+		ng_flags.flags = bencode_dictionary_get(ng_flags.dict, "flags");

Do you really need to create the flags here? Aren't the calling functions creating it?

—
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/4071/review/2506057003@github.com>