Bug #4138
closedlogging_vty_add_cmds parameter remove mess
100%
Description
<pespin> amazing mess. Apparently since March 15th 2017, libosmocore API logging_vty_add_cmds() had its parameter removed, but all our osmocom processes still pass a parameter to it nowadays. Even tests/logging/logging_vty_test.c passes a param to it!.. <pespin> how is it possible that compiler doesn't warn about that? I'm really confused <pespin> it was removed in libosmocore c65c5b4ea075ef6cef11fff9442ae0b15c1d6af7 <tnt> pespin: because it's declared as "void logging_vty_add_cmds();" and not "void logging_vty_add_cmds(void);" <LaF0rge> tnt: ACK. <pespin> ah good point <LaF0rge> pespin: so do you think that extra argument in R0 could screw up anything in the function body? <pespin> LaF0rge, that's first thing I saw and I'm thinking about that possibility yes <pespin> LaF0rge, I think we need to fix programs to remove the extra param, then use (void) in libosmocore <LaF0rge> pespin: I somehow doubt it, more thinking about some thread-local-storage issue, see my recent updates to the related sysmocom ticket <pespin> it's been more than 2 years, so I guess that's fine compatibility-wise
$ ag logging_vty_add_cmds libosmocore/tests/logging/logging_vty_test.c 249: logging_vty_add_cmds(&log_info); libosmocore/tests/vty/vty_test.c 508: logging_vty_add_cmds(); libosmocore/src/vty/logging_vty.c 76: * You have to call \ref logging_vty_add_cmds from your application 331: NULL, /* cmdstr is dynamically set in logging_vty_add_cmds(). */ 385: NULL, /* cmdstr is dynamically set in logging_vty_add_cmds(). */ 994:void logging_vty_add_cmds() libosmocore/include/osmocom/vty/logging.h 9:void logging_vty_add_cmds(); osmo-pcu/src/pcu_vty.c 1167: logging_vty_add_cmds(cat); osmo-trx/Transceiver52M/osmo-trx.cpp 587: logging_vty_add_cmds(); osmo-sip-connector/src/main.c 136: logging_vty_add_cmds(&mncc_sip_info); osmo-hlr/src/hlr_vty.c 434: logging_vty_add_cmds(cat); libosmo-sccp/stp/stp_main.c 180: logging_vty_add_cmds(&log_info); libosmo-sccp/tests/vty/ss7_asp_vty_test.c 165: logging_vty_add_cmds(&log_info); osmo-sgsn/src/gprs/sgsn_main.c 387: logging_vty_add_cmds(NULL); osmo-sgsn/src/gprs/gb_proxy_main.c 287: logging_vty_add_cmds(NULL); osmo-sgsn/src/gprs/gtphub_main.c 359: logging_vty_add_cmds(NULL); osmo-iuh/src/hnbgw.c 542: logging_vty_add_cmds(&hnbgw_log_info); osmo-bsc/src/osmo-bsc/bsc_vty.c 5258: logging_vty_add_cmds(NULL); osmocom-bb/src/host/layer23/src/mobile/app_mobile.c 443: logging_vty_add_cmds(NULL); osmocom-bb/src/shared/libosmocore/src/vty/logging_vty.c 154: NULL, /* cmdstr is dynamically set in logging_vty_add_cmds(). */ 578:void logging_vty_add_cmds(const struct log_info *cat) osmocom-bb/src/shared/libosmocore/include/osmocom/vty/logging.h 8:void logging_vty_add_cmds(const struct log_info *cat); osmo-ggsn/ggsn/ggsn.c 1270: logging_vty_add_cmds(NULL); osmo-msc/src/osmo-msc/msc_main.c 547: logging_vty_add_cmds(&log_info); openbsc/openbsc/src/osmo-bsc_nat/bsc_nat.c 1631: logging_vty_add_cmds(NULL); openbsc/openbsc/src/osmo-bsc_nat/bsc_nat_vty.c 1437: logging_vty_add_cmds(NULL); openbsc/openbsc/src/osmo-bsc_mgcp/mgcp_main.c 225: logging_vty_add_cmds(NULL); openbsc/openbsc/src/libbsc/bsc_vty.c 4338: logging_vty_add_cmds(NULL); osmo-mgw/src/osmo-mgw/mgw_main.c 273: logging_vty_add_cmds(NULL); osmo-bts/src/common/vty.c 1625: logging_vty_add_cmds(cat); osmo-bts/src/osmo-bts-litecell15/misc/lc15bts_mgr.c 304: logging_vty_add_cmds(&mgr_log_info); osmo-bts/src/osmo-bts-sysmo/misc/sysmobts_mgr.c 273: logging_vty_add_cmds(&mgr_log_info); osmo-bts/src/osmo-bts-oc2g/misc/oc2gbts_mgr.c 291: logging_vty_add_cmds(&mgr_log_info); osmo-pcap/src/osmo_client_main.c 189: logging_vty_add_cmds(&log_info); osmo-pcap/src/osmo_server_main.c 202: logging_vty_add_cmds(&log_info);
Updated by pespin over 4 years ago
It seems some programs also have issues with osmo_stats_vty_add_cmds(), which never got a parameter list defined but still not using void, and some callers are passing parameters (wrong!) to it.
libosmocore/src/vty/stats_vty.c:52: * Use \ref osmo_stats_vty_add_cmds once at application start-up to libosmocore/src/stats.c:50: * installed by osmo_stats_vty_Add_cmds(). osmo-pcu/src/pcu_vty.c:1168: osmo_stats_vty_add_cmds(cat); osmo-sip-connector/src/main.c:137: osmo_stats_vty_add_cmds(&mncc_sip_info); osmo-sgsn/src/gprs/gb_proxy_main.c:289: osmo_stats_vty_add_cmds(&gprs_log_info); osmo-sgsn/src/gprs/sgsn_main.c:389: osmo_stats_vty_add_cmds(&gprs_log_info); osmo-ggsn/ggsn/ggsn.c:1272: osmo_stats_vty_add_cmds(&log_info); openbsc/openbsc/src/osmo-bsc_nat/bsc_nat.c:1632: osmo_stats_vty_add_cmds(&log_info); openbsc/openbsc/src/osmo-bsc_mgcp/mgcp_main.c:226: osmo_stats_vty_add_cmds(&log_info); osmo-mgw/src/osmo-mgw/mgw_main.c:275: osmo_stats_vty_add_cmds(&log_info); osmo-pcap/src/osmo_client_main.c:190: osmo_stats_vty_add_cmds(&log_info); osmo-pcap/src/osmo_server_main.c:203: osmo_stats_vty_add_cmds(&log_info);
Updated by pespin over 4 years ago
- Status changed from New to Feedback
- % Done changed from 0 to 90
https://gerrit.osmocom.org/c/libosmocore/+/15038 tests: logging: Remove undefined param passed to logging_vty_add_cmds
https://gerrit.osmocom.org/c/libosmo-sccp/+/15040 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-iuh/+/15041 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-hlr/+/15042 Remove undefined param passed to logging_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-ggsn/+/15043 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-sgsn/+/15044 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-mgw/+/15045 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-msc/+/15046 Remove undefined param passed to logging_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-bsc/+/15047 Remove undefined param passed to logging_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-bts/+/15048 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-pcu/+/15039 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-sip-connector/+/15049 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/osmocom-bb/+/15050 Remove undefined param passed to logging_vty_add_cmds
https://gerrit.osmocom.org/c/osmo-pcap/+/15052 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
https://gerrit.osmocom.org/c/openbsc/+/15054 Remove undefined param passed to {logging,osmo_stats}_vty_add_cmds
Then, 2 commits forcing "void" parameter list on those APIs:
remote: https://gerrit.osmocom.org/c/libosmocore/+/15055 vty: osmo_stats_vty_add_cmds: Enforce no parameters [WIP]
remote: https://gerrit.osmocom.org/c/libosmocore/+/15056 vty: logging_vty_add_cmds: Enforce no parameters [WIP]
Once all are merged, this ticket can be closed.
Updated by pespin over 4 years ago
- Status changed from Feedback to Resolved
- % Done changed from 90 to 100
So the outcome is we don't want to merge last 2 patches for now since it would break backward compatibility with old users which made wrong calls to those APIs.
Closing the ticket.