Attention is currently required from: laforge, fixeria, pespin.
8 comments:
Patchset:
I would probably have used a bool rather than an enum as I see no chance we ever want to deal with a […]
yes but i figured after converting lots of bools to enums, it would be weird to go on to add a new bool again in something that *could* conceptually have more than two preferences...
(new patch set coming up)
File include/osmocom/core/logging.h:
Patch Set #1, Line 400: /* Set timezone for logging of timestamps: 0 for local time, 1 for UTC. Other values are reserved for future
0 and 1? don't we have enums for that in this patch?
right, thx
File src/core/logging.c:
Patch Set #1, Line 474: return 0;
I'd really return an error if the requested timestamp cannot be obtained.
the code before this patch didn't care about the return value of localtime_r(), but if you insist...
Patch Set #1, Line 528: } else if (target->print_timestamp && get_timestamp(&tv, &tm, target) == 0) {
you are calling get_timestamp twice here every time we log something. […]
There is an unlikely corner case where it gets called twice, i.e. only if calling the first time failed. In all practical cases it is called once.
This code changes to a cleaner switch() in upcoming
https://gerrit.osmocom.org/c/libosmocore/+/5857/5/src/core/logging.c#520
Patch Set #1, Line 874: target->timezone = timezone;
Ack
i don't agree. i would not want to duplicate some enum validation code here. It can be assumed that the caller only passes valid enum values, and if it is an unknown enum value, then the logging.c switch() deals with that. Otherwise we always have to keep this function "artificially" updated with what logging.c supports.
File src/vty/logging_vty.c:
Patch Set #1, Line 203: log_set_timezone(tgt, LOG_TIMEZONE_UTC);
Ack
if i can defend above argument, then no here as well =)
File tests/logging/logging_vty_test.vty:
Patch Set #1, Line 75: logging_vty_test# logging timestamp ?
See https://gerrit.osmocom.org/c/libosmocore/+/32044.
pau has a solid point here, i failed to add usability tests for the 'logging timestamp' command itself, which now unconvered that i so far fail to write back the new vty command.
To view, visit change 32043. To unsubscribe, or for help writing mail filters, visit settings.