[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] sysklogd: add -Z option to adjust message timezones
From:       Shiz <hi () shiz ! me>
Date:       2017-05-09 13:13:27
Message-ID: 74EC03EA-520D-4D23-AFB4-52D8ABA62D7B () shiz ! me
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


> On 9 May 2017, at 09:35, walter harms <wharms@bfs.de> wrote:
> 
> Since this is a musl (only ?) feature i would suggest
> to make that selectable.
> 
> re,
> wh

I wouldn't peg this as a musl-only feature — in fact, their argument
seems to be that this is the only way a POSIX-conformant syslog() can
behave, as it would otherwise modify global state (through settz()
through localtime_r()) that it would not be permitted to modify.

Given the very modest code increase I do not particularly see the point
of it being configurable at build time, but if others concur I'll change it.

>> 
>> @@ -815,17 +820,23 @@ static void timestamp_and_log(int pri, char *msg, int len)
>> {
>> 	char *timestamp;
>> 	time_t now;
>> +	struct tm nowtm = { .tm_isdst = 0 };
> why ? is there a bug ?

strptime() may not set the tm_isdst field, as a result of which it would have
an undefined value if the struct is not explicitly initialised, which messes
with the calculation in mktime().

>> 
>> 	/* Jan 18 00:11:22 msg... */
>> 	/* 01234567890123456 */
>> 	if (len < 16 || msg[3] != ' ' || msg[6] != ' '
>> 	 || msg[9] != ':' || msg[12] != ':' || msg[15] != ' '
>> 	) {
>> -		time(&now);
>> +		now = time(NULL);
> 
> is the same

I just figured it's nicer if the code paths look similar.

>> 		timestamp = ctime(&now) + 4; /* skip day of week */
>> 	} else {
>> -		now = 0;
>> -		timestamp = msg;
>> +		if (G.adjustTimezone && strptime(msg, "%b %e %T", &nowtm)) {
>> +			now = mktime(&nowtm) - timezone;
>> +			timestamp = ctime(&now) + 4; /* skip day of week */
>> +		} else {
>> +			now = 0;
>> +			timestamp = msg;
>> +		}
> 
> It is more easy to disable the feature if you do
> 		now = 0;
> 		timestamp = msg;
> 		if (G.adjustTimezone && strptime(msg, "%b %e %T", &nowtm)) {
> 			now = mktime(&nowtm) - timezone;
> 			timestamp = ctime(&now) + 4; /* skip day of week */
> 		}

Right, I'll change that as well if the consensus is that it being selectable
is better.

Thanks for the feedback!

- Shiz

["signature.asc" (signature.asc)]

-----BEGIN PGP SIGNATURE-----

iQIcBAEBCAAGBQJZEcCKAAoJEI8YjKeZk+kHfFwQAKjnRXBANlNGe9TCh5sHaQfZ
CCyYz04rRDaYdhk6kL8fZc9ih+zJYfeFm8gCo/D/P97PYxvMM2ak2+DzjmbIZWMf
q7m/hC3jOpvQnyWg0T4uDv2rz1rK25+ob/G8/FSgpB9Zt2TP8iPBhPNM/l10mASj
FvkFl0rb7/G4GFR+btRQrn/X9IvjdcS6zum9pauPBJYCRMmKKfl064Ai8oal6JUx
ky7FGdMUTUrWkZtxDLPjctRi77lZ2cv7kogR9OW3f9mUqLxiUEy9pKHhWXccoKT5
TOXpV9pAQV+tiP4uIa3ZkwXqOKIUKzlc+I8YanVza1jgbz9BLNd5a3Cm6EeSq6tj
LXo6cfzToapx13hDW+cdaoa/Ec3BERTvIB2qAchHAvDxVJivgBLx3IKF3+EbDQ6H
n5XJvGSrUN73b+Wq7huz6orDIJ6jlgw/cpZGdpd2QGi/QO01gdscEgHpKrmmFXsr
/O6GRMycy1xRF7GjCvUF0fEXZh/uTebF2AIy9xpxLU4sB6g0UWSmm8VmHeYa75uJ
PfBy9CH8q04D0p6BAewOK7x7rH5xrPkvEHLEVhT8ynNVmqEkglSQqEbdCRggd7xP
8MfYKvrriAO/JDOKvdf1iYqxrtyyvZbMNgmZ5pkFCtgUR4S/gUlpDgW6y7uHYiBG
kOzgWT6Ys+qeZMruBl4j
=NiG0
-----END PGP SIGNATURE-----


_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[prev in list] [next in list] [prev in thread] [next in thread]