FreeBSD does not have the daylight global. so I just check tm_is_dst

[madpilot78]

NOTE: I'm the maintainer of the FreeBSD port here:

http://www.freshports.org/finance/gnucash

And I'm going to update the port including this modification. I'd like to upstream it or have suggestions for a better fix.

Thanks in advance.

[jralls]

That won't work, you'll be an hour off during DST. How did the libofx maintainer patch ofxdate_to_time_t(const string ofxdate)?

[madpilot78]

Thanks for the feedback. I suspected my patch was not good.

You can look at the patch here:

https://svnweb.freebsd.org/ports/head/finance/libofx/files/patch-lib_ofx__utilities.cpp?revision=424012&view=markup

which makes me suspect the FreeBSD port of libofx could be unaffected.

[jralls]

Well, not exactly unaffected. That patch is wrong, too. It sets daylight to the current time's is_dst instead of that of the date being calculated, so it will always apply the offset if it's currently DST in the user's locale and always not if it's not. You'll need to either compare the current is_dst with the imported date's is_dst and make the appropriate correction if they differ, or (better IMO) get the libofx maintainer to use my patch from libofx bug 39. With libofx patched configure will detect that bug 39 is fixed and you won't need to patch GnuCash.

[madpilot78]

Great analysis and suggestion.

I'd like to leave this pull request suspended while I contact the libofx port maintainer, I'll followup and close it as soon as I have the issue solved.

Thanks again.

[madpilot78]

I spoke too early.

The patch too libofx you suggest also depends on the daylight variable.

I tried modifying it but the gnucash configuration script detects it as buggy anyway, so my modifications are not correct.

Is there some documentation about this variable? I am not aware of anything equivalent in FreeBSD, so I'm not sure how to work around this issue in libofx or FreeBSD either.

[hgrundy]

Just my two cents, but the existing fix in gnucash here seems like it would break in a few different circumstances... DST being added or removed, DST offset of more or less than one hour, and multiple sets of DST rules will all result in an incorrect time.

The value of "daylight" today doesn't reflect its historic value either, so this may behave strangely in TZs that used to have DST and don't now, or TZs that do have DST now but didn't before.

Do you think it's worth creating a new report for this?

[jralls]

@hgrundy: No, there's no need for another bug report. Your concerns are misplaced.

[jralls]

@madpilot78, "daylight" is an int set by GNU libc's implementation of tzset(3). It indicates whether the timezone currently in effect observes DST, 1 meaning that it does. Not whether DST is currently in effect (as the author of that bit of libofx code thought), just whether it observes it.

The patch uses daylight only to initialize time.tm_isdst before calling checked_mktime(). I'd think that using 0 would work as well. What did you try?

[madpilot78]

@jralls I originally tested your patch to libofx but setting time.tm_isdst = -1, as the FreeBSD man page for mktime [1] says if that value is negative it will try to guess. I also tested 0 as you suggest right now. In both cases gnucash configure script detects it as buggy and enables the correction code.

You can view the patch I've tested here:

http://www.madpilot.net/~mad/libofx.diff

[1] https://www.freebsd.org/cgi/man.cgi?query=mktime&apropos=0&sektion=0&manpath=FreeBSD+11.0-RELEASE&arch=default&format=html

[madpilot78]

I reimplemented the FreeBSD ifdef with a check which should emulate the glibc daylight global.

I'm sorry for the noise, I was trying to rebase this on top of the master branch but I've been unable to do that.

Is the patch acceptable in this way?

[jralls]

@madpilot78 you can't rebase onto master because your branch is based on maint. You need to rebase it onto maint.

Your patch is also defective: The gnc date functions don't use the system ones because on some 32-bit systems time_t is 32 bits and suffers the 2038 bug, so they use GDateTime in maint and boost::datetime in master. Call tzset() to set tzname and then test (tzname && tzname[1]) before dereferencing the latter just to make sure that it won't segfault.

Also, ' ' is not the first character of the empty string, it's '\0', so tzname[1][0] != ' ' would be false only in the extremely unlikely event that the DST string somehow had a leading space.

Finally, stm is the struct tm for the parsed time. You need to make a struct tm for the current time and check is_dst for that in order to reverse the damage done by the BSD packager of libofx.

[madpilot78]

@jralls Regarding the ' ' check, it's there for a reason.

If you look here: https://github.com/freebsd/freebsd/blob/master/contrib/tzcode/stdtime/localtime.c#L77

you'll see that FreeBSD implementation of tzset explicitly sets the TZ abbreviation string to " " when it is not defined, and not to an empty string.

Regarding the other suggestions I'll follow them and add commits here.

Thanks.

[jralls]

@madpilot78 OK, you need a comment saying that so that someone doesn't look at the glibc docs and "fix" it on you.

[jralls]

Uh, no. Study the code some more: That just prevents dereferencing tzname from crashing if tzset() hasn't been called yet. If the TZ doesn't have a DST tzname then the string is set to the empty string just as it is in glibc.

[madpilot78]

(revised comment after your last one)

I performed a few quick tests using a short C snipped and printf, and I have observed it actually returning " ".

Anyway I'll check the code better.

[madpilot78]

Looking again at it I can't see it setting tzname[] to \0.

https://github.com/freebsd/freebsd/blob/master/contrib/tzcode/stdtime/localtime.c#L84

here it clearly points out it will set tzname[1] = " " when timezone is set to a timezone without DST, for the same reason.

EDIT: I'd add that due to the points 3 and 4 in that comment, any case where tzname[] members happen to be set to \0 are to be accounted as bugs in the implemntation (I mean the FreeBSD implementation, where that comment applies)

tzname is filled in settzname(), at line 298. It first sets both tzname[0] and tzname[1] to " ", then fills them in or otherwise does not touch them.

I can be missing something, if that's the case please point my nose at the code line that sets it to \0.

[madpilot78]

I have updated the patch as requested.

In the while I have posted a fix for the FreeBSD libofx port, so maybe I can work with stm directly. I'd wait to see if the patch to libofx port is accepted.

here is the FreeBSD bug report for libofx:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=220665

[jralls]

Ah, perhaps my mistake. I think I misunderstood the behavior of ttisp->tt_isdst. Transitions between two non-DST offsets won't set it and settzname() will just overwrite tzname[0] with the value of the newer transition.

If that's right then if there was ever a DST in the timezone file its name will be in tzname[1], which might be a problem.

But that's all moot. You're trying to undo the behavior of the libofx bug, but BSD has its own different bug. The code from the libofx BSD patch is:

int daylight;

const struct tm* t;
std::time(&temptime);
 t = localtime(&temptime);
daylight = t->tm_isdst;

time.tm_isdst = daylight; // initialize dst setting
 local_offset = difftime(mktime(localtime(&temptime)), mktime(gmtime(&temptime))) + (3600 * daylight);

daylight will be 1 if it is currently DST in the timezone and localtime() applies DST if it's appropriate so adding 3600 * daylight applies an extra hour to the offset. So where the original code added an hour to the offset if the TZ does DST at all, the FreeBSD code adds an extra hour if it's currently DST. If the day on the imported time is a DST day then the offset will be 1 hour off; if it's not a DST day then it will be two hours off.

[madpilot78]

I agree with your analysis, but I have provided a patch to the libofx port which should fix that behavior.

You can look at it at the link I provided.

[jralls]

Yeah, we cross-posted. Aside from the rare case where a TZ had DST once but dumped it (in which case tzname[1] will be set for that ancient DST timezone) that will at least make libofx misbehave the same on FreeBSD as everywhere else.

Post here if the patch gets accepted and I'll merge this.

[madpilot78]

@jralls I have not checked, but I'm sure FreeBSD does not keep any historical timezone data, only current information, so I don't think it's even possible for the OS to return a DST zone if none is catered for by the current rules for the country/area. I could be wrong though, and I would not know a way to work around that either in any case.

[jralls]

On the contrary, it's clear from the code that FreeBSD uses the same NIST time zone files as everyone else but Microsoft.

[madpilot78]

@jralls the patch to libofx was accepted and I also committed this patch as a local patch for gnucash in the FreeBSD port.

Rif.:
freebsd/freebsd-ports@0a0b5b1
freebsd/freebsd-ports@089eab1

I also rebased the pull request to make sure everything is clean.

Thanks!

[gjanssens]

@jralls For when you're back...
I believe all conditions are now met to apply this PR ? I'll let you do the final review though as you have much more in-depth knowledge of time handling in gnucash...

[jralls]

Thanks.