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.

Sign up for GitHub

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

parser: Support 'System' timezone in Time_Offset #7994

Closed
wants to merge 3 commits into from
Closed

parser: Support 'System' timezone in Time_Offset #7994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cef8e2
parsre: fix tzone_offset test
SijmenHuizenga Sep 30, 2023
b7ee1e8
parser: Fix hour validation in timezone offset parsing
SijmenHuizenga Sep 30, 2023
33e624f
parser: Support system timezone in Time_Offset
SijmenHuizenga Sep 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions src/flb_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include <sys/stat.h>
#include <limits.h>
#include <string.h>
#include <time.h>

static inline uint32_t digits10(uint64_t v) {
if (v < 10) return 1;
Expand Down Expand Up @@ -530,7 +531,8 @@ static int parser_conf_file(const char *cfg, struct flb_cf *cf,
name, cfg);
goto fconf_early_error;
}



/* skip_empty_values */
skip_empty = FLB_TRUE;
tmp_str = get_parser_key(config, cf, s, "skip_empty_values");
Expand Down Expand Up @@ -992,6 +994,21 @@ int flb_parser_do(struct flb_parser *parser, const char *buf, size_t length,
return -1;
}

int system_utc_offset(int *tmdiff)
{
#ifdef _WIN32
// Adjust the sign to match tm_gmtoff
*tmdiff = -_timezone;
return 0;
#else
time_t currentTime = time(NULL);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works for timezones that don't have DST; you need to calculate the offset for the timestamp of each record, not just capture the current offset at process start time. Different log entries will have different offsets because of DST.

struct tm localTime = {0};
localtime_r(&currentTime, &localTime);
*tmdiff = localTime.tm_gmtoff;
return 0;
#endif
}

/* Given a timezone string, return it numeric offset */
int flb_parser_tzone_offset(const char *str, int len, int *tmdiff)
{
Expand All @@ -1008,6 +1025,11 @@ int flb_parser_tzone_offset(const char *str, int len, int *tmdiff)
return 0;
}

/* Check system timezones */
if (*p == 'S') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth doing a strncmp here instead of just checking the first character.

return system_utc_offset(tmdiff);
}

/* Unexpected timezone string */
if (*p != '+' && *p != '-') {
*tmdiff = 0;
Expand Down Expand Up @@ -1040,7 +1062,7 @@ int flb_parser_tzone_offset(const char *str, int len, int *tmdiff)
min = ((p[2] - '0') * 10) + (p[3] - '0');
}

if (hour < 0 || hour > 59 || min < 0 || min > 59) {
if (hour < 0 || hour > 23 || min < 0 || min > 59) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

return -1;
}

Expand Down
53 changes: 47 additions & 6 deletions tests/internal/parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,20 @@ struct tz_check tz_entries_ok[] = {
{"+0000", 0},
{"+00:00", 0},
{"+00:59", 3540},
{"-0600", -21000},
{"-06:00", -21000},
{"+23:59", 86340},
{"-0600", -21600},
{"-06:00", -21600},
{"Z", 0},
// System is mocked to timezone "HST" is -10 hours
{"System", -36000},
};

struct tz_check tz_entries_error[] = {
{"0000", 0},
{"+00:90", 0},
{"--600", 0},
{"-06:00", -21000},
{"+24:00", 0},
{"foo", 0},
};

/* Time Lookup */
Expand Down Expand Up @@ -123,6 +128,35 @@ int flb_parser_regex_do(struct flb_parser *parser,
void **out_buf, size_t *out_size,
struct flb_time *out_time);

static char* mock_timezone(char *tz)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nicer if mock_timezone returned an int with success, and took a pointer to where the original timezone should be returned. Then instead of doing perror and exit here, the test can see a -1 from this function and go through the existing acutest failure pattern.

{
char *original_tz = getenv("TZ");
if (original_tz) {
original_tz = strdup(original_tz);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does strduping the string into itself do?

if (!original_tz) {
perror("Failed to allocate memory for original_tz");
exit(EXIT_FAILURE);
}
}

setenv("TZ", tz, 1);
tzset();

return original_tz;
}

static void undo_mock_timezone(char *original_tz)
{
if (original_tz) {
setenv("TZ", original_tz, 1);
} else {
unsetenv("TZ");
}
tzset();
free(original_tz);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to you, but imo it's easier to keep track of frees if they happen in the same scope as the allocation. I think it would make more sense if undo_mock_timezone wasn't in charge of freeing original_tz, and instead the test did this after using it to undo the mock timezone.

}


/* Parse timezone string and get the offset */
void test_parser_tzone_offset()
{
Expand All @@ -132,23 +166,30 @@ void test_parser_tzone_offset()
int diff;
struct tz_check *t;

/* Hawaii Standard Time chosen for not having daylight saving time */
char *original_timezone = mock_timezone("HST");

/* Valid offsets */
for (i = 0; i < sizeof(tz_entries_ok) / sizeof(struct tz_check); i++) {
t = &tz_entries_ok[0];
t = &tz_entries_ok[i];
TEST_CASE(t->val);
len = strlen(t->val);

ret = flb_parser_tzone_offset(t->val, len, &diff);
TEST_CHECK(ret == 0 && diff == t->diff);
TEST_CHECK(ret == 0);
TEST_CHECK_(diff == t->diff, "expected diff %d but got %d", t->diff, diff);
}

/* Invalid offsets */
for (i = 0; i < sizeof(tz_entries_error) / sizeof(struct tz_check); i++) {
t = &tz_entries_error[0];
t = &tz_entries_error[i];
len = strlen(t->val);

ret = flb_parser_tzone_offset(t->val, len, &diff);
TEST_CHECK(ret != 0);
}

undo_mock_timezone(original_timezone);
}

static void load_json_parsers(struct flb_config *config)
Expand Down