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`

-- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/37830?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: I970dccd7a27b098eea9e660822e24e2c4b059fc6 Gerrit-Change-Number: 37830 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Attention: neels <nhofmeyr@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Comment-Date: Wed, 21 Aug 2024 16:36:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> Comment-In-Reply-To: laforge <laforge@osmocom.org> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de>