[PATCH] VTY enable then write crashed gproxy; this patch fixes it.
The previous code made gprs_categories have 18 entries, rather than the 3 a glance would suggest. The first 15 were all-0, as per implicit initialization of static objects in the c standard. This caused problems for code in libosmocore which expected actual data. There are three potential solutions: a) Define a new enum for gprs_categories b) Change libosmocore to not crash on all-zero input categories. c) Simply use log_info, which has a superset of the information. This patch implements c, the simplest. sgsn_main.c had a similar problem; it implicitly had 20 categories. (see readelf -s gprs/osmo-sgsn | grep gprs_categories). --- openbsc/src/gprs/gb_proxy_main.c | 28 ++---------------- openbsc/src/gprs/sgsn_main.c | 62 ++-------------------------------------- 2 files changed, 4 insertions(+), 86 deletions(-) diff --git a/openbsc/src/gprs/gb_proxy_main.c b/openbsc/src/gprs/gb_proxy_main.c index b5b11fd..0a96527 100644 --- a/openbsc/src/gprs/gb_proxy_main.c +++ b/openbsc/src/gprs/gb_proxy_main.c @@ -200,30 +200,6 @@ static struct vty_app_info vty_info = { .is_config_node = bsc_vty_is_config_node, }; -/* default categories */ -static struct log_info_cat gprs_categories[] = { - [DGPRS] = { - .name = "DGPRS", - .description = "GPRS Packet Service", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, - [DNS] = { - .name = "DNS", - .description = "GPRS Network Service (NS)", - .enabled = 1, .loglevel = LOGL_INFO, - }, - [DBSSGP] = { - .name = "DBSSGP", - .description = "GPRS BSS Gateway Protocol (BSSGP)", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, -}; - -static const struct log_info gprs_log_info = { - .filter_fn = gprs_log_filter_fn, - .cat = gprs_categories, - .num_cat = ARRAY_SIZE(gprs_categories), -}; int main(int argc, char **argv) { @@ -239,11 +215,11 @@ int main(int argc, char **argv) signal(SIGUSR2, &signal_handler); osmo_init_ignore_signals(); - osmo_init_logging(&gprs_log_info); + osmo_init_logging(&log_info); vty_info.copyright = openbsc_copyright; vty_init(&vty_info); - logging_vty_add_cmds(&gprs_log_info); + logging_vty_add_cmds(&log_info); gbproxy_vty_init(); handle_options(argc, argv); diff --git a/openbsc/src/gprs/sgsn_main.c b/openbsc/src/gprs/sgsn_main.c index 08e0a85..e5161b4 100644 --- a/openbsc/src/gprs/sgsn_main.c +++ b/openbsc/src/gprs/sgsn_main.c @@ -227,64 +227,6 @@ static void handle_options(int argc, char **argv) } } -/* default categories */ -static struct log_info_cat gprs_categories[] = { - [DMM] = { - .name = "DMM", - .description = "Layer3 Mobility Management (MM)", - .color = "\033[1;33m", - .enabled = 1, .loglevel = LOGL_NOTICE, - }, - [DPAG] = { - .name = "DPAG", - .description = "Paging Subsystem", - .color = "\033[1;38m", - .enabled = 1, .loglevel = LOGL_NOTICE, - }, - [DMEAS] = { - .name = "DMEAS", - .description = "Radio Measurement Processing", - .enabled = 0, .loglevel = LOGL_NOTICE, - }, - [DREF] = { - .name = "DREF", - .description = "Reference Counting", - .enabled = 0, .loglevel = LOGL_NOTICE, - }, - [DGPRS] = { - .name = "DGPRS", - .description = "GPRS Packet Service", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, - [DNS] = { - .name = "DNS", - .description = "GPRS Network Service (NS)", - .enabled = 1, .loglevel = LOGL_INFO, - }, - [DBSSGP] = { - .name = "DBSSGP", - .description = "GPRS BSS Gateway Protocol (BSSGP)", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, - [DLLC] = { - .name = "DLLC", - .description = "GPRS Logical Link Control Protocol (LLC)", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, - [DSNDCP] = { - .name = "DSNDCP", - .description = "GPRS Sub-Network Dependent Control Protocol (SNDCP)", - .enabled = 1, .loglevel = LOGL_DEBUG, - }, -}; - -static const struct log_info gprs_log_info = { - .filter_fn = gprs_log_filter_fn, - .cat = gprs_categories, - .num_cat = ARRAY_SIZE(gprs_categories), -}; - - int main(int argc, char **argv) { struct gsm_network dummy_network; @@ -299,11 +241,11 @@ int main(int argc, char **argv) signal(SIGUSR2, &signal_handler); osmo_init_ignore_signals(); - osmo_init_logging(&gprs_log_info); + osmo_init_logging(&log_info); vty_info.copyright = openbsc_copyright; vty_init(&vty_info); - logging_vty_add_cmds(&gprs_log_info); + logging_vty_add_cmds(&log_info); sgsn_vty_init(); handle_options(argc, argv); -- 1.8.1.2
On Sun, Mar 03, 2013 at 07:15:24AM +0000, Katerina Barone-Adesi wrote: Hi,
This patch implements c, the simplest.
I agree that c) is the best approach. I looked through the commits and LaF0rge changed it from c) to the code as it is now in commit revision: 11461a64574314fbc4747fe6251ca000fdd56b75. LaF0rge do you remember why you used a custom array instead of the commong log_info? thanks holger
Hi Holger and others, On Sun, Mar 03, 2013 at 12:17:58PM +0100, Holger Hans Peter Freyther wrote:
LaF0rge do you remember why you used a custom array instead of the commong log_info?
I think it was my futile attempt at making the unused categories disappear from the "log level <tab>", as explained in my other mail. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi Katerina, On Sun, Mar 03, 2013 at 07:15:24AM +0000, Katerina Barone-Adesi wrote:
The previous code made gprs_categories have 18 entries, rather than the 3 a glance would suggest. The first 15 were all-0, as per implicit initialization of static objects in the c standard. This caused problems for code in libosmocore which expected actual data. There are three potential solutions: a) Define a new enum for gprs_categories b) Change libosmocore to not crash on all-zero input categories. c) Simply use log_info, which has a superset of the information. This patch implements c, the simplest. sgsn_main.c had a similar problem; it implicitly had 20 categories. (see readelf -s gprs/osmo-sgsn | grep gprs_categories).
I was aware of the problem, but never found time for a proper solution. In general, I prefer to have one enum and not program-specific enums, as we tend to move code from one executable to another over tiem and I would like to avoid clashes. The probelm with the current attempt of registering the full log_info is that if you type "logging level <tab>" in the SGSN, it will show you a long list of categories that make absolutely no sense in the context of the SGSN. There is no call control, rsl or lapdm in the context of GPRS. One idea might be to have another member of the struct that allows them to be globally disabled/invisible. This way the SGSN startup code could specify which log categories are actually used, and the VTY strings would be generated only for that sub-set of categories. I'm open for other ideas, too... Regards, Harald -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On Sun, Mar 03, 2013 at 01:50:27PM +0100, Harald Welte wrote:
One idea might be to have another member of the struct that allows them to be globally disabled/invisible. This way the SGSN startup code could specify which log categories are actually used, and the VTY strings would be generated only for that sub-set of categories.
We are currently using a uint8_t for 'enabled' in the logging category struct. We could use some bits for SYS_ENABLED/SYS_DISABLED to either disable (my preference) or enable categories. The logging_vty.c code could take this into account. The danger is that we disable something that should be enabled.
participants (3)
-
Harald Welte -
Holger Hans Peter Freyther -
Katerina Barone-Adesi