pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/15947 )
Change subject: add sdp_msg API: SDP parsing/composition ...................................................................... Patch Set 1: (4 comments) > Patch Set 1: > > doesn't it much rather make sense to have at least a helper library > containing those things that you are copy+pasting here? something like > libosmo-sdp-utils, which then can be used by any of the actual parser > codebases? Copy+Pasting of such code really hurts me :/ Agree. https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/include/osmocom/msc/debug.h File include/osmocom/msc/debug.h: https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/include/osmocom/msc/debug.h@38 PS1, Line 38: size_t len = 64; \ Makes much more sense having len as a param to avoid almost-sure talloc_realloc on some functions. https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/include/osmocom/msc/debug.h@40 PS1, Line 40: char *str = (char*)talloc_named_const(ctx, len, #FUNC_BUF "_c()"); \ where does this "ctx" var come from? https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/include/osmocom/msc/debug.h@63 PS1, Line 63: #define NAME_IMPL(FUNC_C, FUNC_C_ARGS...) \ So you return a talloc'ed pointer on success and a static string pointer on error? That will break when you attempt to free it. I'd drop this function completely, null is your friend! https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/src/libmsc/sdp_msg.c File src/libmsc/sdp_msg.c: https://gerrit.osmocom.org/c/osmo-msc/+/15947/1/src/libmsc/sdp_msg.c@105 PS1, Line 105: codec->fmtp[0] ? codec->fmtp : NULL); shouldn't you compare against '\0' here? codec->fmtp[0] != '\0' ? ... -- To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15947 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Change-Id: If3ce23cd5bab15e2ab4c52ef3e4c75979dffe931 Gerrit-Change-Number: 15947 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-CC: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 04 Nov 2019 10:38:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
