Attention is currently required from: laforge, neels.
fixeria has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/osmo-upf/+/37830?usp=email )
Change subject: add VTY 'gtp-echo' command ......................................................................
Patch Set 2:
(2 comments)
File src/osmo-upf/upf_vty.c:
https://gerrit.osmocom.org/c/osmo-upf/+/37830/comment/4b09c019_20fbc45f?usp=... : PS1, Line 491: tx to
Would you accept this? `gtp-echo tx remote-ip IP_ADDR [(local-ip|local-dev)] [IP_ADDR_OR_DEV]`
The problem with this all-in-one command is that the syntax is misleading: you're adding two optional parts (`(local-ip|local-dev)` and `IP_ADDR_OR_DEV`), which are in fact one optional part. The command syntax suggests that you may pass only the `(local-ip|local-dev)` part and the command would be accepted, but the actual command logic will throw an error. This is why I suggested to **define them as separate DEFUNs**.
What if we want to allow resolving a hostname [...]
Ok, I see why you're specifically prefixing it with `remote-ip`. However you're not limiting the value format by using `VTY_IPV46_CMD`, so the user can still pass non-IP values like `gtp-echo tx remote-ip google.com`.
I think you can define it as `gtp-echo tx VTY_IPV46_CMD ...` for now and change to `"gtp-echo tx (RHOST | " VTY_IPV4_CMD "|" VTY_IPV6_CMD ") ..."` in case you want to add hostname resolution.
I can't imagine (maybe due to limited vision) what else we may want to do with GTP ECHO from the `ENABLE_NODE` (not the config) other than sending it, but I understand the problem and generally fine keeping the `tx` part. Maybe change it to `send` to be more user-friendly.
Just sharing my view here. You have CR+1 from Harald. If others are fine with the proposed command syntax, than feel free to merge the patch as-is. It's unlikely that I am going to use this command a lot during my usual development anyway ;)
https://gerrit.osmocom.org/c/osmo-upf/+/37830/comment/0dab7412_784fe107?usp=... : PS1, Line 556: %%Error
missing space?
yes, between the `%%` and `Error`
-
fixeria