plugins:rtc.c: drop some invalid condition checks #453
Conversation
troglobit
left a comment
troglobit
left a comment
There was a problem hiding this comment.
Interesting PR, I have some considerations posted in line.
liuming50
force-pushed
the
fix-rtc-restore-issues
branch
from
41d9e7a to
5b7eef7
Compare
|
Hi, @troglobit I have made a V2 based on your comments and my replies, please kindly help me review it, I am now doing some runtime tests on it. |
troglobit
left a comment
troglobit
left a comment
There was a problem hiding this comment.
Please continue simplifying this, after all, it's PID 1 so the code needs to be readable and easy to audit. Thanks for hanging in there!
liuming50
force-pushed
the
fix-rtc-restore-issues
branch
from
5b7eef7 to
5d444b7
Compare
|
Please help me review the V3, I have done some runtime tests with it, at the first boot, when the rtc time is too old and the restore file not present yet, the output is like the following: |
troglobit
left a comment
troglobit
left a comment
There was a problem hiding this comment.
Looks really good, I love how you extended the status output! Much better from a usability and user experience point of view.
Only one small comment left, nice work!
plugins/rtc.c
Outdated
| print_desc(NULL, "Restoring system time from timestamp"); | ||
| rc = time_set(NULL); |
There was a problem hiding this comment.
Small nit, replace "Restoring system time from timestamp" with same text as we use in the log message in time_set() for NULL argument:
logit(LOG_NOTICE, "Resetting system clock to kernel default, %s.", rtc_timestamp);I think that would look nicer and easy to also find in the logs if needed.
There was a problem hiding this comment.
@troglobit yes that can be better, will send a V4 soon.
There are some invalid condition checks in rtc.c, when rtc_file is missing, it should not consider that as a error and return, otherwise, it can lead to the case that time_set wont be called at all. With this fix, when both RTC and file restore fail, it will fall back to call time_set(NULL) and restore time from rtc_timestamp, this ensures the OS has a valid time at the very first boot. Signed-off-by: Ming Liu <[email protected]>
liuming50
force-pushed
the
fix-rtc-restore-issues
branch
from
5d444b7 to
13a8f56
Compare
|
I have made a V4 according to your suggestion. |
troglobit
left a comment
troglobit
left a comment
There was a problem hiding this comment.
Awesome, very nice work on this!
2 participants
There are some invalid condition checks in rtc.c, when rtc_file is missing, it should not consider that as a error and return, otherwise, it can lead to the case that time_set wont be called at all.
With this fix, when both RTC and file restore fail, it will fall back to call time_set(NULL) and restore time from rtc_timestamp, this ensures the OS has a valid time at the very first boot.
Also explicitly check strptime return value against "NULL" rather than using "!", which is not always accurate.