Update patch set 3

Patch Set 3: Code-Review-1

(3 comments)

Patch-set: 3
Label: Code-Review=-1
This commit is contained in:
Pau Espin Pedrol 2017-09-25 14:16:46 +00:00 committed by Gerrit Code Review
parent 33f13a42de
commit 5373f9d044
1 changed files with 55 additions and 0 deletions

View File

@ -0,0 +1,55 @@
{
"comments": [
{
"key": {
"uuid": "9aa7fdbe_67878d62",
"filename": "src/libbsc/bsc_vty.c",
"patchSetId": 3
},
"lineNbr": 317,
"author": {
"id": 1000074
},
"writtenOn": "2017-09-25T14:16:46Z",
"side": 1,
"message": "Make sure bts-\u003euptime 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-\u003euptime. So, as conclusion, one of the two parts of the code is wrong.\nI 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\u0027t 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.\n\n\nStrictly speaking I am not sure it\u0027s actually good using difftime for this, because according to man page it expects calendar clocks and afaik MONOTONIC clock doesn\u0027t follow that rule. Anyway, implementation wise it\u0027s only substracting one with another which seems like the expected behaviour.",
"revId": "46d0d183568de623d8584c6dfe836f3be371fc9c",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326",
"unresolved": false
},
{
"key": {
"uuid": "9aa7fdbe_67a06df5",
"filename": "src/libbsc/bsc_vty.c",
"patchSetId": 3
},
"lineNbr": 318,
"author": {
"id": 1000074
},
"writtenOn": "2017-09-25T14:16:46Z",
"side": 1,
"message": "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.\n\nfunction timespec_to_elapsed(char *buf, timespec *tp) or similar?",
"revId": "46d0d183568de623d8584c6dfe836f3be371fc9c",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326",
"unresolved": false
},
{
"key": {
"uuid": "9aa7fdbe_c7959989",
"filename": "src/libbsc/bsc_vty.c",
"patchSetId": 3
},
"lineNbr": 319,
"author": {
"id": 1000074
},
"writtenOn": "2017-09-25T14:16:46Z",
"side": 1,
"message": "may be nice to also move all this operations to different macros in the same header file? may come handy later in other parts.",
"revId": "46d0d183568de623d8584c6dfe836f3be371fc9c",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326",
"unresolved": false
}
]
}