Attention is currently required from: osmith, fixeria, pespin, msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31513 ) Change subject: GSMTAP: allow configuring src IP for log messages ...................................................................... Patch Set 4: (9 comments) Patchset: PS4: it would be great to also see this tested in logging_vty_test.vty: i know it sounds annoying, but i've made so many mistakes already, merging broken vty commands i wrote ... now my opinion is that the only sane way to add vty commands is to add a *.vty test that verifies the doc strings are right and the config is written back ok. This patch affects all osmo programs so it's worth being tested. so ... please add to tests/logging/logging_vty_test.vty lines like this: # log? # log ? # log gsmtap ? # log gsmtap foo ? # log gsmtap foo 127.0.0.99 # show running-config ... log gsmtap foo 127.0.0.99 ... literally like above, and then you can run make vty-test U=-u to automagically fill in what the vty returns File src/core/logging_gsmtap.c: https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/dd43a8ff_ce234187 PS4, Line 122: g (for doxygen: the summary line should end in a period '.') It would be nice to point out the reason for version 2 to exist like Same as log_target_create_gsmtap() with the possibility to provide a local IP and port to send gsmtap from. https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/7beb53bb_610dbc32 PS4, Line 123: m or NULL for "localhost". https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/1636f6b6_dedaaba5 PS4, Line 124: m or zero for any local port. https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/41bc5041_a5741028 PS4, Line 126: o or 0 to use GSMTAP_UDP_PORT. (this one was also "missing" in the old doc, but might as well add it) https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/98edbb78_1eaf7339 PS4, Line 156: src src can be passed as NULL, should we call talloc_strdup() on NULL? File src/vty/logging_vty.c: https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/67bece04_ffc31c91 PS4, Line 808: ) (i guess it should say "default is 127.0.0.1" but yes, not really part of this patch) https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/773c0977_13fc80b1 PS4, Line 822: NULL rather "any" .. at least some string literal instead of passing NULL to varargs, so that we have a definite deterministic string to test for in vty transcript testing if we ever want to File utils/osmo-stat-dummy/osmo-stat-dummy.cfg: https://gerrit.osmocom.org/c/libosmocore/+/31513/comment/bc72d7ab_0338f759 PS4, Line 1: log gsmtap 127.0.0.69 127.6.9.1 what's the rationale for adding this here at all? don't we have some example cfg file for logging? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31513 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I9000269ce5b3dce1e757271b7c42e77b68d38f25 Gerrit-Change-Number: 31513 Gerrit-PatchSet: 4 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 01:32:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment