@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.
> + 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?
> + 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` ?
> @@ -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 or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4071#pullrequestreview-2506057003
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4071/review/2506057...@github.com>_______________________________________________
Kamailio - Development Mailing List -- sr-dev@lists.kamailio.org
To unsubscribe send an email to sr-dev-le...@lists.kamailio.org
Important: keep the mailing list in the recipients, do not reply only to the
sender!
