Project

General

Profile

Bug #4138

logging_vty_add_cmds parameter remove mess

Added by pespin 2 months ago. Updated 10 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
libosmocore
Target version:
-
Start date:
07/31/2019
Due date:
% Done:

100%

Spec Reference:

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);

History

#1 Updated by pespin 2 months ago

  • Description updated (diff)

#2 Updated by pespin 2 months 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);

#3 Updated by pespin 2 months 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.

#4 Updated by pespin 10 days 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.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)