<p style="white-space: pre-wrap; word-wrap: break-word;">tldr: yes, but let's discuss UI vs implementation quirks with pmaier first.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/10022">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c">File src/libosmo-mgcp/mgcp_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@193">Patch Set #1, Line 193:</a> <code style="font-family:monospace,monospace"> vty_out(vty, "Endpoint %s0x%.2x:%s",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Interesting, what does the '.2' do for %x?</p><p style="white-space: pre-wrap; word-wrap: break-word;">edit 1: Ah I see now you're just moving previous code. These remain interesting questions but not related to this patch then.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@201">Patch Set #1, Line 201:</a> <code style="font-family:monospace,monospace"> /* FIXME: Also add verbosity for other</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use the line width instead of breaking lines?</p><p style="white-space: pre-wrap; word-wrap: break-word;">edit: also just moved code</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@256">Patch Set #1, Line 256:</a> <code style="font-family:monospace,monospace"> "Display information about an MGCP Media Gateway endpoint\n"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>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)</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p><p style="white-space: pre-wrap; word-wrap: break-word;">(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?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10022/1/src/libosmo-mgcp/mgcp_vty.c@280">Patch Set #1, Line 280:</a> <code style="font-family:monospace,monospace"> vty_out(vty, "endpoint name '%s' is not a hex number%s", argv[1], VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(re "rtpbridge/2@mgw" above)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/10022">change 10022</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/10022"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I5330e697ec34bf215de91d44209048a8dc226d51 </div>
<div style="display:none"> Gerrit-Change-Number: 10022 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 19 Jul 2018 00:02:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>