Re: x11/blackbox: time_t fix (too late i see .. but please read)
On 8/18/13, patrick keshishian sids...@boxsoft.com wrote:
Swaping accounts for sending diff in-line and not have
gmail fuck it up.
On Sat, Aug 17, 2013 at 20:03:32 -0600, Theo de Raadt wrote:
So basically, this takes a long long value, and casts it to a long.
Yes, but that's not the entire story.
the function declaration is: long nextTimeout(int resolution)
So it would truncate anyway. But look at it closer. It
takes the value of timeval.tv_sec (your time_t / long long)
and mod's it with resolution (an int value). The only thing
it then does is multiply it by 1000L.
The only reason this function broke is C++'s std::max()
defined as a template function, requiring both args be of
the same typename. Since the original code uses 1000L
literal for the first arg, the compiler was having issues with
the long long as the second arg., but ...
What is long long % int? another int value? Yes, I realize
it would be a long long according to the compiler, but,
I fail to see where/how 0x % 0xFF would
ever be greater than 0xfe.
As is, the best you can expect from that function is a long
second argument for std::max(). Unless I'm completely missing
something here (and I'll certainly budget for that possibility).
You're completely missing the point.
We provide an opportunity to the entire community to fix things
in the best way possible.
And you squander it.
The solution is to take all the subsystems and arguments to time_t,
throughout the entire program, and then do the best from there.
Instead, you chose to prematurely downcast.
Now, before you say this is impossible or ridiculous, let me say this.
We did what I suggest to the entire base tree, before the kernel
changes were commited. If we can do this to the entire base tree
system -- something much much larger than the average package, then
people working in the ports tree can at least try to not squander
the opportunity.
Please thanks.
unsquandering ... (hopefully)
The reason for the extra REVISION bump is for ajacoutot@'s
patch-src_Toolbar_cc addition (it missed the bump?).
Minimally tested.
well?
Also, would like to (again) bring attention to this patch:
http://marc.info/?l=openbsd-portsm=137339931231574w=2
--patrick
Index: Makefile
===
RCS file: /cvs/obsd/ports/x11/blackbox/Makefile,v
retrieving revision 1.51
diff -u -p -u -p -r1.51 Makefile
--- Makefile 21 Mar 2013 08:48:55 - 1.51
+++ Makefile 18 Aug 2013 07:14:22 -
@@ -1,9 +1,9 @@
-# $OpenBSD: Makefile,v 1.50 2013/03/11 11:46:08 espie Exp $
+# $OpenBSD: Makefile,v 1.51 2013/03/21 08:48:55 ajacoutot Exp $
COMMENT= small pretty window manager for 8 and more bits displays
DISTNAME=blackbox-0.70.1
-REVISION=2
+REVISION=4
CATEGORIES= x11
HOMEPAGE=http://blackboxwm.sourceforge.net/
Index: patches/patch-lib_Resource_cc
===
RCS file: patches/patch-lib_Resource_cc
diff -N patches/patch-lib_Resource_cc
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Resource_cc 18 Aug 2013 07:14:22 -
@@ -0,0 +1,20 @@
+$OpenBSD$
+
+64bit time_t fix.
+
+--- lib/Resource.cc.orig Wed Apr 6 14:16:50 2005
lib/Resource.cc Sat Aug 17 22:26:11 2013
+@@ -207,6 +207,13 @@ void bt::Resource::write(const char* resource,
unsigne
+ }
+
+
++void bt::Resource::write(const char* resource, unsigned long long value)
{
++ char tmp[64];
++ sprintf(tmp, %llu, value);
++ write(resource, tmp);
++}
++
++
+ void bt::Resource::write(const char* resource, bool value)
+ { write(resource, boolAsString(value)); }
+
Index: patches/patch-lib_Resource_hh
===
RCS file: patches/patch-lib_Resource_hh
diff -N patches/patch-lib_Resource_hh
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Resource_hh 18 Aug 2013 07:14:22 -
@@ -0,0 +1,14 @@
+$OpenBSD$
+
+64bit time_t fix.
+
+--- lib/Resource.hh.orig Mon Jan 3 01:42:52 2005
lib/Resource.hh Sat Aug 17 22:25:59 2013
+@@ -78,6 +78,7 @@ namespace bt {
+ void write(const char* resource, unsigned int value);
+ void write(const char* resource, long value);
+ void write(const char* resource, unsigned long value);
++void write(const char* resource, unsigned long long value);
+ void write(const char* resource, bool value);
+ void write(const char* resource, double value);
+
Index: patches/patch-lib_Timer_cc
===
RCS file: patches/patch-lib_Timer_cc
diff -N patches/patch-lib_Timer_cc
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Timer_cc18 Aug 2013 07:14:22 -
@@ -0,0 +1,25 @@
Re: x11/blackbox: time_t fix (too late i see .. but please read)
Attaching a new diff which includes both sets of patches mentioned in this thread. As a bonus, includes two new patch files to silence warnings about deprecated conversion from string constant to 'char*'. --patrick blackbox.diff Description: Binary data
Re: x11/blackbox: time_t fix (too late i see .. but please read)
Swaping accounts for sending diff in-line and not have
gmail fuck it up.
On Sat, Aug 17, 2013 at 20:03:32 -0600, Theo de Raadt wrote:
So basically, this takes a long long value, and casts it to a long.
Yes, but that's not the entire story.
the function declaration is: long nextTimeout(int resolution)
So it would truncate anyway. But look at it closer. It
takes the value of timeval.tv_sec (your time_t / long long)
and mod's it with resolution (an int value). The only thing
it then does is multiply it by 1000L.
The only reason this function broke is C++'s std::max()
defined as a template function, requiring both args be of
the same typename. Since the original code uses 1000L
literal for the first arg, the compiler was having issues with
the long long as the second arg., but ...
What is long long % int? another int value? Yes, I realize
it would be a long long according to the compiler, but,
I fail to see where/how 0x % 0xFF would
ever be greater than 0xfe.
As is, the best you can expect from that function is a long
second argument for std::max(). Unless I'm completely missing
something here (and I'll certainly budget for that possibility).
You're completely missing the point.
We provide an opportunity to the entire community to fix things
in the best way possible.
And you squander it.
The solution is to take all the subsystems and arguments to time_t,
throughout the entire program, and then do the best from there.
Instead, you chose to prematurely downcast.
Now, before you say this is impossible or ridiculous, let me say this.
We did what I suggest to the entire base tree, before the kernel
changes were commited. If we can do this to the entire base tree
system -- something much much larger than the average package, then
people working in the ports tree can at least try to not squander
the opportunity.
Please thanks.
unsquandering ... (hopefully)
The reason for the extra REVISION bump is for ajacoutot@'s
patch-src_Toolbar_cc addition (it missed the bump?).
Minimally tested.
Index: Makefile
===
RCS file: /cvs/obsd/ports/x11/blackbox/Makefile,v
retrieving revision 1.51
diff -u -p -u -p -r1.51 Makefile
--- Makefile21 Mar 2013 08:48:55 - 1.51
+++ Makefile18 Aug 2013 07:14:22 -
@@ -1,9 +1,9 @@
-# $OpenBSD: Makefile,v 1.50 2013/03/11 11:46:08 espie Exp $
+# $OpenBSD: Makefile,v 1.51 2013/03/21 08:48:55 ajacoutot Exp $
COMMENT= small pretty window manager for 8 and more bits displays
DISTNAME= blackbox-0.70.1
-REVISION= 2
+REVISION= 4
CATEGORIES=x11
HOMEPAGE= http://blackboxwm.sourceforge.net/
Index: patches/patch-lib_Resource_cc
===
RCS file: patches/patch-lib_Resource_cc
diff -N patches/patch-lib_Resource_cc
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Resource_cc 18 Aug 2013 07:14:22 -
@@ -0,0 +1,20 @@
+$OpenBSD$
+
+64bit time_t fix.
+
+--- lib/Resource.cc.orig Wed Apr 6 14:16:50 2005
lib/Resource.ccSat Aug 17 22:26:11 2013
+@@ -207,6 +207,13 @@ void bt::Resource::write(const char* resource, unsigne
+ }
+
+
++void bt::Resource::write(const char* resource, unsigned long long value) {
++ char tmp[64];
++ sprintf(tmp, %llu, value);
++ write(resource, tmp);
++}
++
++
+ void bt::Resource::write(const char* resource, bool value)
+ { write(resource, boolAsString(value)); }
+
Index: patches/patch-lib_Resource_hh
===
RCS file: patches/patch-lib_Resource_hh
diff -N patches/patch-lib_Resource_hh
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Resource_hh 18 Aug 2013 07:14:22 -
@@ -0,0 +1,14 @@
+$OpenBSD$
+
+64bit time_t fix.
+
+--- lib/Resource.hh.orig Mon Jan 3 01:42:52 2005
lib/Resource.hhSat Aug 17 22:25:59 2013
+@@ -78,6 +78,7 @@ namespace bt {
+ void write(const char* resource, unsigned int value);
+ void write(const char* resource, long value);
+ void write(const char* resource, unsigned long value);
++void write(const char* resource, unsigned long long value);
+ void write(const char* resource, bool value);
+ void write(const char* resource, double value);
+
Index: patches/patch-lib_Timer_cc
===
RCS file: patches/patch-lib_Timer_cc
diff -N patches/patch-lib_Timer_cc
--- /dev/null 1 Jan 1970 00:00:00 -
+++ patches/patch-lib_Timer_cc 18 Aug 2013 07:14:22 -
@@ -0,0 +1,25 @@
+$OpenBSD$
+
+64bit time_t fix.
+
+--- lib/Timer.cc.orig Fri Mar 18 01:07:09 2005
lib/Timer.cc Sat Aug 17 22:43:13 2013
+@@ -28,8 +28,7 @@
+
+
+ bt::timeval::timeval(const ::timeval t)
+- : tv_sec(t.tv_sec),
Re: x11/blackbox: time_t fix (too late i see .. but please read)
patrick keshishian sids...@boxsoft.com wrote:
unsquandering ... (hopefully)
I think most of this is pointless and misleading. The timeout value
that is passed around is NOT a time_t. It's a time offset in the
range 0 .. 3600*1000 milliseconds. Putting it into a time_t is an
abuse of that type.
+--- lib/Timer.hh.origFri Mar 18 01:07:09 2005
lib/Timer.hh Sat Aug 17 22:43:03 2013
+@@ -37,16 +37,12 @@ struct timeval;
+ namespace bt {
+
+ // use a wrapper class to avoid the header as well
+- struct timeval {
+-long tv_sec;
+-long tv_usec;
++ struct timeval : public ::timeval {
Now THIS is a good catch.
--
Christian naddy Weisgerber na...@mips.inka.de
Re: x11/blackbox: time_t fix (too late i see .. but please read)
I think most of this is pointless and misleading. The timeout value that is passed around is NOT a time_t. It's a time offset in the range 0 .. 3600*1000 milliseconds. Putting it into a time_t is an abuse of that type. However, typecasting a time_t to an int is almost always an error. Unless justified.
Re: x11/blackbox: time_t fix (too late i see .. but please read)
On 8/18/13, Christian Weisgerber na...@mips.inka.de wrote:
patrick keshishian sids...@boxsoft.com wrote:
unsquandering ... (hopefully)
I think most of this is pointless and misleading. The timeout value
that is passed around is NOT a time_t. It's a time offset in the
range 0 .. 3600*1000 milliseconds. Putting it into a time_t is an
abuse of that type.
I agree. The purpose of nextTimeout(), seems to be, to returning
an interval in milliseconds. That is then passed to Timer::setTimeout().
--patrick
+--- lib/Timer.hh.orig Fri Mar 18 01:07:09 2005
lib/Timer.hhSat Aug 17 22:43:03 2013
+@@ -37,16 +37,12 @@ struct timeval;
+ namespace bt {
+
+ // use a wrapper class to avoid the header as well
+- struct timeval {
+-long tv_sec;
+-long tv_usec;
++ struct timeval : public ::timeval {
Now THIS is a good catch.
--
Christian naddy Weisgerber na...@mips.inka.de
Re: x11/blackbox: time_t fix (too late i see .. but please read)
On Sun, Aug 18, 2013 at 2:43 AM, patrick keshishian sids...@boxsoft.com wrote:
+ bt::timeval::timeval(const ::timeval t)
+- : tv_sec(t.tv_sec), tv_usec(t.tv_usec)
+-{ }
++{ tv_sec = t.tv_sec; tv_usec = t.tv_usec; }
What's the point of these changes?
Re: x11/blackbox: time_t fix (too late i see .. but please read)
On 8/18/13, Matthew Dempsky matt...@dempsky.org wrote:
On Sun, Aug 18, 2013 at 2:43 AM, patrick keshishian sids...@boxsoft.com
wrote:
+ bt::timeval::timeval(const ::timeval t)
+- : tv_sec(t.tv_sec), tv_usec(t.tv_usec)
+-{ }
++{ tv_sec = t.tv_sec; tv_usec = t.tv_usec; }
What's the point of these changes?
The bt::timeval struct's definition changed. tv_sec and tv_usec are
no longer direct members, and can not be initialized as before.
--patrick
x11/blackbox: time_t fix (too late i see .. but please read)
Hi,
I see naddy@ already commited a fix for this from NetBSD. Thank
you!
My patch fwiw was to cast second arg to std::max() to long.
$OpenBSD$
--- src/Toolbar.cc.orig Sat Aug 17 16:58:14 2013
+++ src/Toolbar.cc Sat Aug 17 17:08:12 2013
@@ -44,7 +44,7 @@ long nextTimeout(int resolution)
{
timeval now;
gettimeofday(now, 0);
- return (std::max(1000l, resolution - (now.tv_sec % resolution)) * 1000l))
+ return (std::max(1000l, (long)resolution - (now.tv_sec %
resolution)) * 1000l))
- (now.tv_usec / 1000l;
}
anyway, while on topic of x11/blackbox, i posted a patch on July 9th
to fix title bar issue with UTF8 sequence. See details in my original
post:
http://marc.info/?l=openbsd-portsm=137339931231574w=2
Can I ask someone to review/commit this patch as well?
Cheers,
--patrick
Re: x11/blackbox: time_t fix (too late i see .. but please read)
I see naddy@ already commited a fix for this from NetBSD. Thank
you!
My patch fwiw was to cast second arg to std::max() to long.
$OpenBSD$
--- src/Toolbar.cc.orig Sat Aug 17 16:58:14 2013
+++ src/Toolbar.ccSat Aug 17 17:08:12 2013
@@ -44,7 +44,7 @@ long nextTimeout(int resolution)
{
timeval now;
gettimeofday(now, 0);
- return (std::max(1000l, resolution - (now.tv_sec % resolution)) *
1000l))
+ return (std::max(1000l, (long)resolution - (now.tv_sec %
resolution)) * 1000l))
- (now.tv_usec / 1000l;
}
So basically, this takes a long long value, and casts it to a long.
So, on a 32 bit machine, that throws away the high bits.
Meaning it will make it compile today, but it will break in 2038.
So, let me summarize. the transition to 'long long' exposes a real
problem. The addition of this (long) cast now hides the problem; so
that sometime further into the future someone is going to have a
harder time fixing it right.
That is wrong.
Re: x11/blackbox: time_t fix (too late i see .. but please read)
On 8/17/13, Theo de Raadt dera...@cvs.openbsd.org wrote:
I see naddy@ already commited a fix for this from NetBSD. Thank
you!
My patch fwiw was to cast second arg to std::max() to long.
$OpenBSD$
--- src/Toolbar.cc.orig Sat Aug 17 16:58:14 2013
+++ src/Toolbar.cc Sat Aug 17 17:08:12 2013
@@ -44,7 +44,7 @@ long nextTimeout(int resolution)
{
timeval now;
gettimeofday(now, 0);
- return (std::max(1000l, resolution - (now.tv_sec % resolution)) *
1000l))
+ return (std::max(1000l, (long)resolution - (now.tv_sec %
resolution)) * 1000l))
- (now.tv_usec / 1000l;
}
So basically, this takes a long long value, and casts it to a long.
Yes, but that's not the entire story.
the function declaration is: long nextTimeout(int resolution)
So it would truncate anyway. But look at it closer. It
takes the value of timeval.tv_sec (your time_t / long long)
and mod's it with resolution (an int value). The only thing
it then does is multiply it by 1000L.
The only reason this function broke is C++'s std::max()
defined as a template function, requiring both args be of
the same typename. Since the original code uses 1000L
literal for the first arg, the compiler was having issues with
the long long as the second arg., but ...
What is long long % int? another int value? Yes, I realize
it would be a long long according to the compiler, but,
I fail to see where/how 0x % 0xFF would
ever be greater than 0xfe.
As is, the best you can expect from that function is a long
second argument for std::max(). Unless I'm completely missing
something here (and I'll certainly budget for that possibility).
--patrick
So, on a 32 bit machine, that throws away the high bits.
Meaning it will make it compile today, but it will break in 2038.
So, let me summarize. the transition to 'long long' exposes a real
problem. The addition of this (long) cast now hides the problem; so
that sometime further into the future someone is going to have a
harder time fixing it right.
That is wrong.
Re: x11/blackbox: time_t fix (too late i see .. but please read)
So basically, this takes a long long value, and casts it to a long. Yes, but that's not the entire story. the function declaration is: long nextTimeout(int resolution) So it would truncate anyway. But look at it closer. It takes the value of timeval.tv_sec (your time_t / long long) and mod's it with resolution (an int value). The only thing it then does is multiply it by 1000L. The only reason this function broke is C++'s std::max() defined as a template function, requiring both args be of the same typename. Since the original code uses 1000L literal for the first arg, the compiler was having issues with the long long as the second arg., but ... What is long long % int? another int value? Yes, I realize it would be a long long according to the compiler, but, I fail to see where/how 0x % 0xFF would ever be greater than 0xfe. As is, the best you can expect from that function is a long second argument for std::max(). Unless I'm completely missing something here (and I'll certainly budget for that possibility). You're completely missing the point. We provide an opportunity to the entire community to fix things in the best way possible. And you squander it. The solution is to take all the subsystems and arguments to time_t, throughout the entire program, and then do the best from there. Instead, you chose to prematurely downcast. Now, before you say this is impossible or ridiculous, let me say this. We did what I suggest to the entire base tree, before the kernel changes were commited. If we can do this to the entire base tree system -- something much much larger than the average package, then people working in the ports tree can at least try to not squander the opportunity. Please thanks.
