Doesn’t build on FreeBSD releases branch due to daylight variable from time.h #43

cochard commented 2024年09月22日 11:50:08 +02:00

Describe the bug

ipmitool 1.8.19 do not build on FreeBSD older that the upcoming devel-branch 15.
So on all release (13 and 14) the build fails with this error:

ipmi_time.c:108:3: error: use of undeclared identifier 'daylight'
 daylight = -1;
 ^

it seems this daylight variable isn’t correctly exported in the include time.h.

IPMITOOL Version

1.8.19

To Reproduce

Just building on FreeBSD.
We’ve "fixed" this problem by deleting the culprid line.
https://cgit.freebsd.org/ports/commit/?id=643f82bb3aee9d2d137390054012a65030dcb849

But this is not the correct way to fix this problem.

The build is working on the development branch because of this commit in FreeBSD time.h that is hidding daylight in a __XSI_VISIBLE condition:
https://cgit.freebsd.org/src/commit/?id=a34940a9756ac8edce36fec176949ee82e9235b4

But I have no idea of how to fix this cleanly on release 13 and 14.

**Describe the bug** ipmitool 1.8.19 do not build on FreeBSD older that the upcoming devel-branch 15. So on all release (13 and 14) the build fails with this error: ``` ipmi_time.c:108:3: error: use of undeclared identifier 'daylight' daylight = -1; ^ ``` it seems this daylight variable isn’t correctly exported in the include time.h. **IPMITOOL Version** 1.8.19 **To Reproduce** Just building on FreeBSD. We’ve "fixed" this problem by deleting the culprid line. https://cgit.freebsd.org/ports/commit/?id=643f82bb3aee9d2d137390054012a65030dcb849 But this is not the correct way to fix this problem. The build is working on the development branch because of this commit in FreeBSD time.h that is hidding daylight in a __XSI_VISIBLE condition: https://cgit.freebsd.org/src/commit/?id=a34940a9756ac8edce36fec176949ee82e9235b4 But I have no idea of how to fix this cleanly on release 13 and 14.

AlexanderAmelkin commented 2025年02月07日 20:29:01 +01:00

Looks like __XSI_VISIBLE needs to be defined before inclusion of time.h on FreeBSD.
I don't have FreeBSD, so you're welcome to create a patch and submit it as a PR (please refer to this page in the commit log).

Looks like `__XSI_VISIBLE` needs to be defined before inclusion of `time.h` on FreeBSD. I don't have FreeBSD, so you're welcome to create a patch and submit it as a PR (please refer to this page in the commit log).

AlexanderAmelkin commented 2025年10月15日 17:21:13 +02:00

Did you check if my suggestion helps?

Did you check if my suggestion helps?

Matlib commented 2025年10月25日 23:22:16 +02:00

No, there's no such symbol in FreeBSD < 15.

$ grep -r daylight /usr/include/
/usr/include/sys/clock.h: * daylight savings time, leap-years and such. All that is theoretically a

The following change produces the same error:

diff --git a/lib/ipmi_time.c b/lib/ipmi_time.c
index aecd656..ca5227f 100644
--- a/lib/ipmi_time.c
+++ b/lib/ipmi_time.c
@@ -31,6 +31,7 @@
 * OF SUCH DAMAGES.
 */
 
+#define __XSI_VISIBLE 1
 #include <time.h>
 #include <stdbool.h>
 #include <stdint.h>

The correct way to do this is to set tm.tm_isdst to -1 rather than relaying on some linuxism, and that's how it's done at line 52 in the same file.

No, there's no such symbol in FreeBSD < 15. ``` $ grep -r daylight /usr/include/ /usr/include/sys/clock.h: * daylight savings time, leap-years and such. All that is theoretically a ``` The following change produces the same error: ```diff diff --git a/lib/ipmi_time.c b/lib/ipmi_time.c index aecd656..ca5227f 100644 --- a/lib/ipmi_time.c +++ b/lib/ipmi_time.c @@ -31,6 +31,7 @@ * OF SUCH DAMAGES. */ +#define __XSI_VISIBLE 1 #include <time.h> #include <stdbool.h> #include <stdint.h> ``` The correct way to do this is to set `tm.tm_isdst` to -1 rather than relaying on some linuxism, and that's how it's done at line 52 in the same file.

Matlib commented 2025年10月26日 00:23:13 +02:00

...and resetting tm_isdst is only needed for convertion back into the timestamp with mktime(). The code works fine without that line at all.

diff --git a/lib/ipmi_time.c b/lib/ipmi_time.c
index aecd656..b72f0f9 100644
--- a/lib/ipmi_time.c
+++ b/lib/ipmi_time.c
@@ -105,7 +105,6 @@ ipmi_strftime(char *s, size_t max, const char *format, time_t stamp)
 * the timezone offset.
 */
 gmtime_r(&stamp, &tm);
- daylight = -1;
 } else {
 /*
 * The user wants the time reported in local time zone.

Output:

$ sudo ./ipmitool dcmi power reading
 Instantaneous power reading: 97 Watts
 Minimum during sampling period: 81 Watts
 Maximum during sampling period: 135 Watts
 Average power reading over sample period: 98 Watts
 IPMI timestamp: 10/26/2025 02:18:19 CEST
 Sampling period: 01815155 Seconds.
 Power reading state is: activated
$ sudo ./ipmitool -Z dcmi power reading
 Instantaneous power reading: 98 Watts
 Minimum during sampling period: 81 Watts
 Maximum during sampling period: 135 Watts
 Average power reading over sample period: 98 Watts
 IPMI timestamp: 10/26/2025 00:18:26 UTC
 Sampling period: 01815162 Seconds.
 Power reading state is: activated

...and resetting <i>tm_isdst</i> is only needed for convertion back into the timestamp with mktime(). The code works fine without that line at all. ```diff diff --git a/lib/ipmi_time.c b/lib/ipmi_time.c index aecd656..b72f0f9 100644 --- a/lib/ipmi_time.c +++ b/lib/ipmi_time.c @@ -105,7 +105,6 @@ ipmi_strftime(char *s, size_t max, const char *format, time_t stamp) * the timezone offset. */ gmtime_r(&stamp, &tm); - daylight = -1; } else { /* * The user wants the time reported in local time zone. ``` Output: ``` $ sudo ./ipmitool dcmi power reading Instantaneous power reading: 97 Watts Minimum during sampling period: 81 Watts Maximum during sampling period: 135 Watts Average power reading over sample period: 98 Watts IPMI timestamp: 10/26/2025 02:18:19 CEST Sampling period: 01815155 Seconds. Power reading state is: activated $ sudo ./ipmitool -Z dcmi power reading Instantaneous power reading: 98 Watts Minimum during sampling period: 81 Watts Maximum during sampling period: 135 Watts Average power reading over sample period: 98 Watts IPMI timestamp: 10/26/2025 00:18:26 UTC Sampling period: 01815162 Seconds. Power reading state is: activated ```

Matlib commented 2025年11月17日 18:58:10 +01:00

This is just totally wrong:

		t = time(NULL);
		/*
		 * Now we have local time in t, but BMC requires UTC
		 */
		t = ipmi_localtime2utc(t);
	}
}
...
time_t
ipmi_localtime2utc(time_t local)
{
	struct tm tm;
	gmtime_r(&local, &tm);
	tm.tm_isdst = (-1);
	return mktime(&tm);
}

time_t contains timestamp not broken down date, and it doesn't store any timezone information to convert.

This is just totally wrong: ```c t = time(NULL); /* * Now we have local time in t, but BMC requires UTC */ t = ipmi_localtime2utc(t); } } ... time_t ipmi_localtime2utc(time_t local) { struct tm tm; gmtime_r(&local, &tm); tm.tm_isdst = (-1); return mktime(&tm); } ``` `time_t` contains timestamp not broken down date, and it doesn't store any timezone information to convert.

AlexanderAmelkin commented 2025年12月26日 10:11:24 +01:00

You're correct in that time_t contains timestamp. However, you're misunderstanding what's going on there. The timestamp you get from time(NULL) is the timestamp since Epoch in your local time zone. That is, the start of Epoch in your local timezone is offset from the start of Epoch in Greenwich. Unless of course your local time zone is Greenwich. Most BMCs however store time in UTC. That's why we need to convert your local timestamp to UTC timestamp. That's what ipmi_localtime2utc() does.

You're correct in that `time_t` contains timestamp. However, you're misunderstanding what's going on there. The timestamp you get from `time(NULL)` is the timestamp since Epoch in your local time zone. That is, the start of Epoch in your local timezone is offset from the start of Epoch in Greenwich. Unless of course your local time zone is Greenwich. Most BMCs however store time in UTC. That's why we need to convert your local timestamp to UTC timestamp. That's what `ipmi_localtime2utc()` does.

AlexanderAmelkin commented 2025年12月26日 10:23:08 +01:00

It appears that you're correct though in regard to uselessness of that daylight = -1 assignment. The function tried to mimic the behavior of tzset() on Linux, but apparently nothing in ipmitool relies on that side effect and checks the value of that variable. I suppose it can quite safely be removed.

It appears that you're correct though in regard to uselessness of that `daylight = -1` assignment. The function tried to mimic the behavior of `tzset()` on Linux, but apparently nothing in `ipmitool` relies on that side effect and checks the value of that variable. I suppose it can quite safely be removed.

Matlib commented 2025年12月26日 16:05:18 +01:00

2 - The description of daylight in Linux man page is somewhat cryptic, but I believe it indicates whether current local time is in DST or not. In other words it's ment to be read from, and not written to.

1 - No.
https://linux.die.net/man/2/time "time() returns the time as the number of seconds since the Epoch, 1970年01月01日 00:00:00 +0000 (UTC)"
https://man.freebsd.org/cgi/man.cgi?time(3) "The time() function returns the value of time in seconds since 0 hours, 0 minutes, 0 seconds, January 1, 1970, Coordinated Universal Time (UTC)."

To verify:

#include <stdlib.h>#include <stdio.h>#include <time.h>int main ()
{
	time_t t = time (NULL);
	char s [128];
	strftime (s, sizeof (s), "%c %z", localtime (&t));
	printf ("%ld %s\n", t, s);
	setenv ("TZ", "UTC", 1);
	tzset ();
	t = time (NULL);
	strftime (s, sizeof (s), "%c %z", localtime (&t));
	printf ("%ld %s\n", t, s);
	return 0;
}

$ TZ=Asia/Tokyo ./test 
1766761365 Sat Dec 27 00:02:45 2025 +0900
1766761365 Fri Dec 26 15:02:45 2025 +0000

Also:
https://www.gnu.org/software/freeipmi/freeipmi-faq.html#Why-are-times-reported-by-FreeIPMI-tools-wrong_003f
"Whether or not a system truly stored the timestamps in localtime varies on many factors, such as the vendor, BIOS, and operating system."

2 - The description of `daylight` in Linux man page is somewhat cryptic, but I believe it indicates whether <i>current</i> local time is in DST or not. In other words it's ment to be read from, and not written to. 1 - No. https://linux.die.net/man/2/time <i>"time() returns the time as the number of seconds since the Epoch, 1970年01月01日 00:00:00 +0000 (UTC)"</i> https://man.freebsd.org/cgi/man.cgi?time(3) <i>"The time() function returns the value of time in seconds since 0 hours, 0 minutes, 0 seconds, January 1, 1970, Coordinated Universal Time (UTC)."</i> To verify: ```c #include <stdlib.h> #include <stdio.h> #include <time.h> int main () { time_t t = time (NULL); char s [128]; strftime (s, sizeof (s), "%c %z", localtime (&t)); printf ("%ld %s\n", t, s); setenv ("TZ", "UTC", 1); tzset (); t = time (NULL); strftime (s, sizeof (s), "%c %z", localtime (&t)); printf ("%ld %s\n", t, s); return 0; } ``` ``` $ TZ=Asia/Tokyo ./test 1766761365 Sat Dec 27 00:02:45 2025 +0900 1766761365 Fri Dec 26 15:02:45 2025 +0000 ``` Also: https://www.gnu.org/software/freeipmi/freeipmi-faq.html#Why-are-times-reported-by-FreeIPMI-tools-wrong_003f <i>"Whether or not a system truly stored the timestamps in localtime varies on many factors, such as the vendor, BIOS, and operating system."</i>

AlexanderAmelkin commented 2026年02月26日 19:39:11 +01:00

Oh, my bad. You're correct regarding the nature of value returned by time().

Please check if the code from #80 works for you. It's also very interesting if your BMC supports the SEL Time UTC Offset commands.
I've tried on Dell, Lenovo and Huawei, as well as on an OpenBMC-based server, and neither one did support those commands, so despite your late comment regarding BMC time being local, ipmitool will still treat the SEL time as UTC, because that's usually the case. However, if a BMC supports those commands, it should now be properly taken in account. Can't verify though.

Oh, my bad. You're correct regarding the nature of value returned by `time()`. Please check if the code from #80 works for you. It's also very interesting if your BMC supports the SEL Time UTC Offset commands. I've tried on Dell, Lenovo and Huawei, as well as on an OpenBMC-based server, and neither one did support those commands, so despite your late comment regarding BMC time being local, `ipmitool` will still treat the SEL time as UTC, because that's usually the case. However, if a BMC supports those commands, it should now be properly taken in account. Can't verify though.

Matlib commented 2026年05月08日 23:24:05 +02:00

Sorry for the delay. I've been busy with other things and ipmitool stopped compiling with --disable-intf-lan. To be investigated later.

The code works on one of our servers:

...
Assuming BMC time is UTC
05/08/26 23:48:32 CEST

Sorry for the delay. I've been busy with other things and ipmitool stopped compiling with --disable-intf-lan. To be investigated later. The code works on one of our servers: ``` ... Assuming BMC time is UTC 05/08/26 23:48:32 CEST ```