Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/10022 )
Change subject: add a VTY command which shows a specific mgcp endpoint ...................................................................... Patch Set 1: Code-Review-1 (4 comments) tldr: yes, but let's discuss UI vs implementation quirks with pmaier first. https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c File src/libosmo-mgcp/mgcp_vty.c: https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@193 PS1, Line 193: vty_out(vty, "Endpoint %s0x%.2x:%s", not sure if we should include a 0x here (I remember there has been confusion around decimal vs hex on that topic before, and I used to argue for a 0x in log output; which doesn't really make sense to me anymore). Rather stay with the exact syntax sent via MGCP? Interesting, what does the '.2' do for %x? edit 1: Ah I see now you're just moving previous code. These remain interesting questions but not related to this patch then. edit 2: wait no, you are actually changing the format to prepend "rtpbridge/". So again yes, with this change let's copy the code from actual MGCP message composition? (it's sent as character string) https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@201 PS1, Line 201: /* FIXME: Also add verbosity for other use the line width instead of breaking lines? edit: also just moved code https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@256 PS1, Line 256: "Display information about an MGCP Media Gateway endpoint\n" Common prefix with other commands, here 'show mgcp', should share similar doc strings. This means that here it should also say "..about the MGCP Media Gateway" like above, and in fact it's a candidate for a common #define like the SHOW_STR. What is the trunk number for the user? is it what follows "rtpbridge/" in the endpoint name we got from the MGW? No, that's part of the endpoint NAME, is it?? Looks to me like also allowing to omit 'endpoint NAME' would be good, to show one entire trunk? and/or to allow passing an endpoint name without having to pass a trunk number? (I think I'm confused on what a trunk is) e.g. when reading osmo-bsc logs, AFAICT an endpoint name looks like "rtpbridge/2@mgw", I think we should use similar syntax here to cut out conversions that the user needs to understand to use this command. The command should be so that we can seamlessly add e1 endpoints in the future, hiding weird implementation details (like a hex number) we might have at the moment. Would be nice if pmaier could say something here. The idea: just printing out a listing is less critical than exposing VTY commands; we should try to not require to incompatibly change the vty command UI in the future. So we may prefer to not glorify the weird hex number as proper VTY UI, when the MGCP protocol only ever has character strings. Am I making sense? (also thinking, connections on each endpoint have long random generated ids, CI, extremely likely to be unique across all the MGW. Could be nice to be able to query by CI without having to also pass the endpoint name? oh well, maybe not that useful, querying by endpoint name should suffice. What does pmaier think?) https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@280 PS1, Line 280: vty_out(vty, "endpoint name '%s' is not a hex number%s", argv[1], VTY_NEWLINE); (re "rtpbridge/2@mgw" above) -- To view, visit https://gerrit.osmocom.org/10022 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5330e697ec34bf215de91d44209048a8dc226d51 Gerrit-Change-Number: 10022 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Sperling <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Comment-Date: Thu, 19 Jul 2018 00:02:19 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
