-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
initial freebsd support #607
base: master
Are you sure you want to change the base?
Conversation
Please do not merge until given ok by Saroj or Ashok. |
no, I won't @palladia. I don't have permissions. Who would be good person to talk to make this more polished? e.g. fix packaging and documentation. I tried to look around but I want to make sure it would meet expectation. |
struct tm gmt; | ||
struct tm local; | ||
|
||
time_t now = time(NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is now use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at CPU_GetLocalTimestampz() @JumpingYang001. On Linux, timezone is extern long with offset to GMT. BSD does not have that. (or OSX/MacOS) Since I failed to figure out what we do for Mac, I simply calculate once difference between GMT and local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I found there is a char* timezone() function in FreeBSD.
I think OSX should use exist logic, I don't know much about how BSD dealt with it.
I just surprise the now
variable here isn't used?
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch. there were two lines missing - I probably somehow nuke them in final cleanup. I also add more comments. I hope the code makes more sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, we don't have verification for u.timestamp.utc
in https://github.com/Microsoft/omi/blob/eb689db8a63de6e466f149cd80136de26c53b40c/Unix/tests/pal/test_pal.cpp#L1021 , if you can add one that would be nice, e.g. TEST_ASSERT(now.u.timestamp.utc>= "2018/12/12 00:00AM UTC"'s utc);
, just an example that means it will be greater than a recent past utc time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if my condition is good, just a thought. :)
@wfurt Saroj seems be in vacation today and tomorrow, he might be back next week, let's wait his comments, thanks. |
related to #606. This is initial support without packaging or documentation.
With this change it is possible build and run omiserver and use cli.
I'm able to run all tests.
and this on my Azure machine