Skip to content
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

timeutils refactor #2483

Merged
merged 44 commits into from Mar 7, 2019

Conversation

@bazsi
Copy link
Collaborator

commented Jan 7, 2019

This branch is a second set of patches to refactor time handling code in syslog-ng to be much more readable and easier to change.

The branch achieves a number of things:

  • it splits out timeutils.c to a set of independent modules in the lib/timeutils directory
  • it adds a thin abstraction layer around "struct tm" called WallClockTime
  • it also introduces UnixTime that is a more generic version of the previous LogStamp structure
  • it centralizes all functions around the new abstractions and refactors all users to use the new interfaces (templates, date-parser, snmptrapd-header-parser, syslog-format)
  • contains a number of bugfixes

Bugfixes:

  • the P_ macros were using an invalid value in case the PROCESSED timestamp was not initialized (which is not before serialization)
  • Pythonv2 we were using the GMT as timezone, instead of the local timezone

I still want to add low-level unit tests, so far I was only moving code around and we have some coverage through the existing test suite. It would be much better to test the functionality through the low-level interfaces.

I am opening this PR to show how it is progressing and to solicit feedback.

I have now added unit tests for UnixTime and WallClockTime With those this PR should be complete.

PS: the patchset seems huge, but it is broken down to tiny little patches and each should only make one thing that is trivial to review. I was making an effort to even compile patches one-by-one, which I may have broken in the last round of rebases, but should be easy to fix.

@lbudai lbudai added the in progress label Jan 7, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Build FAILURE

@bazsi bazsi changed the title timeutils refactor WIP: timeutils refactor Jan 8, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

Build FAILURE

@szemere szemere self-requested a review Jan 9, 2019

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch 2 times, most recently from 738f454 to 95a5e02 Jan 19, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

Build FAILURE

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch 2 times, most recently from af06a47 to e73b6cb Jan 19, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

Build FAILURE

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from e73b6cb to b5a8682 Jan 19, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2019

Build SUCCESS

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from b5a8682 to 6b1fd19 Jan 21, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

Build SUCCESS

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from 6b1fd19 to 422c8f7 Jan 21, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

Build SUCCESS

@bazsi bazsi changed the title WIP: timeutils refactor timeutils refactor Jan 21, 2019

@lbudai
Copy link
Contributor

left a comment

Nice refactor!

( one additional note: I think one step is missing, or at least not very comfortable to read: LogStamp is a type alias for UnixTime and it is little bit confusing when a method receives LogStamp and a UnixTime method is applied for that instance... . I think it would be more clean if we would use UnixTime everywhere, if possible. )

Why the request change? Need feedback from @pzoleex and/or @mitzkia if all the internal tests has been passed.

lib/timeutils/wallclocktime.h Show resolved Hide resolved
lib/logmsg/tests/test_logmsg_serialize.c Outdated Show resolved Hide resolved
lib/timeutils/cache.h Outdated Show resolved Hide resolved
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

Build FAILURE

@mitzkia

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

@kira-syslogng retest this please

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Build SUCCESS

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from bf3f546 to a98f79e Jan 23, 2019

@bazsi

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2019

I have squashed all changes back to the original commits.

There's still one open item, the one to eliminate LogStamp in favour of UnixTime. That I would do in the last round of patches, the one that will also do functional changes (e.g. the original aim of guessing the timezone value for incoming timestamps).

So right now we are:

  1. waiting for one more review
  2. and to check if this broke any of the broader functional tests.
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Build SUCCESS

@bazsi

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 24, 2019

I have squashed all changes back to the original commits.

There's still one open item, the one to eliminate LogStamp in favour of UnixTime. That I would do in the last round of patches, the one that will also do functional changes (e.g. the original aim of guessing the timezone value for incoming timestamps).

So right now we are:

  1. waiting for one more review
  2. and to check if this broke any of the broader functional tests.
lib/timeutils/wallclocktime.h Outdated Show resolved Hide resolved
lib/timeutils/wallclocktime.h Outdated Show resolved Hide resolved
lib/timeutils/unixtime.c Outdated Show resolved Hide resolved
lib/timeutils/cache.h Show resolved Hide resolved
lib/timeutils/unixtime.c Outdated Show resolved Hide resolved
@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

Build FAILURE

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from ab51f9b to 0b59635 Jan 26, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2019

Build FAILURE

bazsi added 12 commits Jan 28, 2019
date-parser: refactor to use WallClockTime
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: add normalized versions of the unix_time_set_from_wall_clo…
…ck_time() functions

The algorithm that takes a WallClockTime and converts it to UnixTime
will mutate the WallClockTime, contrary to intuitions. This patch
introduces a second set of set_from() functions, so we have two sets:

1) one that takes a const WallClockTime
2) one that mutates its argument

The second one is qualified with the word "normalized".

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
logstamp: move the legacy LogStamp code to the UnixTime module
This patch deprecates LogStamp and changes all references to UnixTime and
related functions.

The patch is _just_ a refactor, so no functional changes are expected
or intended.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
test_msgparse: get rid off the determine_year_for_month() dependency
Instead of dynamically calculating timestamps based on the current year,
just fake the current time and make the testing simpler.

The determine_year_for_month() function is tested through the new timeutils
unit tests.

All _get_epoch_with_bsd_year() calls in the tests were replaced as if
the current time was in 2019 and the time is faked accordingly.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: make determine_year_for_month() a private function
This function was too low level anyway and we have a higher level
of abstraction with WallClockTime.

This patch also gets rid of the unit test that was covering this function
in favour of the tests in test_wallclocktime.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: move format functions to a separate module
The prototypes of these functions are not yet changed, except for naming,
which is not ideal, as we should conform to a common pattern there.

But the patch is large enough already, so I just renamed the functions
for now.

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: add WallClockTime specific formatter functions
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
template: use the WallClockTime based formatting functions to avoid d…
…ouble-conversion

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: split timeutils.h
This patch splits up timeutils.h into two headers:
1) the 'root' header for the timeutils directory that contains declarations
used accross the timeutils directory

2) the misc.h header containing a couple of unsorted functions

Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
timeutils: rename time conversion functions to be unlike setters
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>
astylerc: add dbld to the list of excludes
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>

@bazsi bazsi force-pushed the bazsi:guess-timezone-based-on-difference2 branch from 2c17c06 to a6f4f34 Feb 25, 2019

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Build SUCCESS

@Kokan Kokan removed this from the OSE 3.20 milestone Feb 28, 2019

@furiel
Copy link
Contributor

left a comment

Thank you for the changes.

test_msgparse: add testcase for checking the "unset" time
Signed-off-by: Balazs Scheidler <balazs.scheidler@oneidentity.com>

@lbudai requested review to indicate that internal tests need to pass. Removing the "request changes" status as those have passed and a number of review rounds were performed already.

@bazsi

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2019

@furiel was kind enough to do a thorough review, we now need another reviewer. All known issues were addressed in the branch and it would be appreciated if I could move forward with the patches I have piled on top of this.

@kira-syslogng

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Build SUCCESS

@furiel
furiel approved these changes Mar 4, 2019

@Kokan Kokan assigned Kokan and unassigned Kokan Mar 5, 2019

@Kokan Kokan self-requested a review Mar 5, 2019

#include "compat/time.h"
#include "timeutils/timeutils.h"
#include "timeutils/wallclocktime.h"
#include "timeutils/unixtime.h"

This comment has been minimized.

Copy link
@szemere

szemere Mar 7, 2019

Contributor

This header is not needed.

@@ -0,0 +1,31 @@
/*

This comment has been minimized.

Copy link
@szemere

szemere Mar 7, 2019

Contributor

As a result of the - VERY appreciated - conv.c/h modification this file became futhile. (If I see it correctly, it is already removed from compiling, only the file left in the repo.)

@szemere
szemere approved these changes Mar 7, 2019

@szemere szemere merged commit 5f8e351 into balabit:master Mar 7, 2019

4 checks passed

Kira-starter Build SUCCESS
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.