Attention is currently required from: laforge, pespin, fixeria.

View Change

1 comment:

• File src/core/logging.c:

  • Patch Set #1, Line 874: target->timezone = timezone;

    Going for the bool value local vs. UTC might avoid the entire isue altogether. […]

    The bool vs enum aspect doesn't really affect the aspect raised by pespin, does it?

    Zooming out a bit, the main purpose of this patch is to add a handful of vty script to logging_vty_test.vty. Maybe the complexity is outgrowing the benefit, and we should simply not test the timestamp format in a .vty test? Then we can skip this API entirely.

    All log_set_..() functions so far just set a config option and return void. It would be a first to add a rc here.

    Comparing to log_set_category_filter(): if the caller passes an invalid category there, we OSMO_ASSERT() and quit the program.

    These ideas:

    If the selected function (gmtime_r() or localtime_r()) was not supported at compile time, then at runtime, one of:

    • OSMO_ASSERT(false) in logging.c when logging happens
    • omit timestamps in log output
    • vty_out("err") and return VTY_CMD_WARNING so starting the application fails.

    I'd argue for: don't show timestamps. Simple and least adverse effects.
    Is this important enough for pivoting CN component operation on that?

    I don't like to add a switch() here that mimicks the switch() in logging.c because it is code dup:

     switch (target->timezone) {
    	case LOG_TIMEZONE_LOCALTIME:
     #ifdef HAVE_LOCALTIME_R
    		return 0;
     #endif
    	case LOG_TIMEZONE_UTC:
     #ifdef HAVE_GMTIME_R
    		return 0;
     #endif
    	default:
    		return -1;
     }

To view, visit change 32043. To unsubscribe, or for help writing mail filters, visit settings.