[sr-dev] Re: [kamailio/kamailio] rtpengine: export subscribe operation functions (PR #4071)

@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. + 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? + 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` ? } /* 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/2506057003(a)github.com>