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;

    Maybe it's a good thing to not add this API at all? Being able to have a regression test for logging that is a central interface towards human users for *all* of our osmo programs is a good reason to go the extra mile. Then again if it just works, then we need no regression test, and no added timezone API.

    what do you think?

    If we decide to keep this, this is my response:

    I made my argument and seems you disagree, but apparenty you completely ignore my arguments and make me restate them:

    Yes, we could add additional validation during log_set_timestamp() and during cfg file reading, if there is a good reason. But we do not *need* to care whether a given timezone function is available. If the system cannot generate a timestamp the way it is configured, then we just don't print one in the logs. No need to abort the program or to fail a cfg file: the program can work safely either way. That's a single #if HAVE_LOCALTIME in one place, and that's it. It is *far* less complex code than what you propose:

    I don't really understand why you are even thinking about asserting or removing timestamps during logging when you can simply have a VTY command fail to set a given timestamp format when you try to set it, as per what I proposed

    We don't need to be strict because the program can still run fine.
    That is also how the other log_set_foo() functions seem to work.

    Code dup: the reason why a given timezone may not be working is hidden in logging.c's log string generation code. I do not want to duplicate that to the function that sets the flag. When the logging output code changes, we will forget to also change the flag setting function accordingly.

    So in summary, you want me to

    • introduce code dup to
    • add non-conforming API behavior, just to
    • make it harder to launch an osmo program on a platform with no localtime_r() function.
    • I say, let's have simpler code, simpler starting of the program, simpler future code maintenance. The user will notice when there are no timestamps, or will just not care. either way is fine.

    In our code we have lots of blocks like below,
    and then one of them suddenly needs its rc checked?

      log_set_print_foo(1);
      log_set_this(1);
      log_set_that(1);
      
      if (log_set_timestamp(xx)) {
          exit(1);
      }
      
    We can do this for a very good reason, but IMHO there just is no good reason.

    Finally, this patch is so unimportant compared to other things, so i don't feel like expanding this discussion needlessly, but here we are.

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