Patch Set 3: Code-Review-1 (3 comments)
https://gerrit.osmocom.org/#/c/4009/3/src/libbsc/bsc_vty.c File src/libbsc/bsc_vty.c: Line 317: sec = (unsigned long long)difftime(tp.tv_sec, bts->uptime); Make sure bts->uptime is also taken from MONOTONIC clock, otherwise you can get strange values with difftime. You should definetly not mix times using different clock sources. Specially because later on, you use ctime() with bts->uptime. So, as conclusion, one of the two parts of the code is wrong. I think it makes sense to use a monotonic clock to print the elapsed time, but you should be using a wall clock to print the ctime part, because a monotonic clock doesn't need to contain similar values than a wall clock (which would print really weird dates). This way it can also be seen easily if there has been some type of big clock drift and understand better the real time it has been up. Strictly speaking I am not sure it's actually good using difftime for this, because according to man page it expects calendar clocks and afaik MONOTONIC clock doesn't follow that rule. Anyway, implementation wise it's only substracting one with another which seems like the expected behaviour. Line 318: vty_out(vty, " %llu days %llu hours %llu min. %llu sec. since %s", All this code in here looks like a good candidate to be moved to its own function. libosmocore ./include/osmocom/core/timer.h (or a new time_util.h) may be a good idea. function timespec_to_elapsed(char *buf, timespec *tp) or similar? Line 319: (sec % (60 * 60 * 24 * 365)) / (60 * 60 * 24), may be nice to also move all this operations to different macros in the same header file? may come handy later in other parts. -- To view, visit https://gerrit.osmocom.org/4009 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4e8504afe8ca467b68d41826f61654e24d9600 Gerrit-PatchSet: 3 Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Owner: Max <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-HasComments: Yes
