Project

General

Profile

Bug #2577

Ensure meaningful default loglevels.

Added by dexter over 2 years ago. Updated about 11 hours ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
10/17/2017
Due date:
% Done:

90%

Spec Reference:

Description

There are some loglevels configured to LOGL_DEBUG by default in osmo-sgsn. Find meaningful defaults and also check the other products.

(We should also add a test that checks if some application registers odd log levels by default.)

History

#1 Updated by laforge 9 months ago

  • Assignee set to lynxis

#2 Updated by laforge 12 days ago

  • Assignee changed from lynxis to dexter

#3 Updated by dexter 7 days ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 50

To ensure proper default loglevel I think it makes sense to print a message as a reminder. Maybe later when we fixed the problem in every osmocom project, then we can migrate to an OSMO_ASSERT()

https://gerrit.osmocom.org/c/libosmocore/+/16832 logging.c: Add valuestrings for loglevels
https://gerrit.osmocom.org/c/libosmocore/+/16833 application.c: check default loglevels on startup

#4 Updated by pespin 5 days ago

I think it makes much more sense to test this as a unit test for apps where we really want to avoid DEBUG being default. I (or someone else) may be writing an app which uses DEBUG by default for whatever reason, and we must not reject that.

#5 Updated by laforge 5 days ago

I would think it's sufficient to go through all applications (it's not that many, after all) and ensure that the current code doesn't have any compiled-in default log level below NOTICE or INFO. In the future we should pay attention during code review to prevent new such bugs from being introduced. I would expect they date back to our pre-gerrit development days.

This way thre's no need for enforcement in libosmocore, which may create problems as pau indicates.

#6 Updated by dexter 4 days ago

  • % Done changed from 50 to 90

I have now checked the applications. LOGL_NOTICE seems to be the standard everywhere. I have found some odd loglevels in osmocom-bb and osmo-bsc which i have corrected. There is also some old stuff bs_11_config and openBSC which I did not touch.

#7 Updated by fixeria about 12 hours ago

Looking at the commits related to this ticket, I noticed that we're basically changing all logging categories to use LOGL_NOTICE. In the end I see an array of struct log_info_cat where every entry has the same line: '.loglevel = LOGL_NOTICE'. This looks odd and cumbersome, so I've got an idea.

LOGL_DEBUG has the lowest value, and it starts from 1:

/*! different log levels */
#define LOGL_DEBUG      1       /*!< debugging information */
#define LOGL_INFO       3       /*!< general information */
#define LOGL_NOTICE     5       /*!< abnormal/unexpected condition */
#define LOGL_ERROR      7       /*!< error condition, requires user action */
#define LOGL_FATAL      8       /*!< fatal, program aborted */

this means that we can omit those repeating lines, and leave it up to log_init() to initialize the default logging level:

diff --git a/src/logging.c b/src/logging.c
index 4d6224d5..60719d50 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -1056,6 +1056,8 @@ int log_init(const struct log_info *inf, void *ctx)
                for (i = 0; i < inf->num_cat; i++) {
                        memcpy((struct log_info_cat *) &osmo_log_info->cat[i],
                               &inf->cat[i], sizeof(struct log_info_cat));
+                       if (!inf->cat[i].loglevel)
+                               !inf->cat[i].loglevel = LOGL_NOTICE;
                }
        }

#8 Updated by dexter about 11 hours ago

I like the idea, lets integrate that.

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)