Re: apr_time_t -- apr_time_usec_t
Justin Erenkrantz wrote: Ryan Bloom wrote: This is going to break EVERY apr app out there. I have no problem with the change, but everything is going to be broken. Is there any way to do this by creating an apr_time32_t, or something that will keep the default at 64-bit, and allow people to use 32 if they specify it? How about using the value we already have: typedef apr_int32_t apr_short_interval_time_t; And, since there *are* processors that are 64-bit out there, I'm not entirely sure if it makes sense for us to go back to 32-bit for performance reasons. -- justin I think it's better to maintain the microseconds part, but to split the seconds and microseconds into separate fields in a timeval structure. Sometimes we actually want microseconds. They're useful, for example, when generating unique IDs or when doing custom performance monitoring. Most of the time, in the httpd at least, we just want seconds. We get both for free from gettimeofday(), so why not keep both? Also, apr_short_interval_time_t isn't really a good choice for holding a time_t. That signed 32-bit int is going to overflow in 2038. --Brian
Re: apr_time_t -- apr_time_usec_t
At 05:04 PM 6/10/2002, you wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. I must have missed that discussion traveling. Pointers please? apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) As far as adopting apr_time_sec_t throughout, you may be looking forward to your retirement party before a signed 32 bit apr_time_sec_t blows chunks. Having coded against Y2K since 1989, I'd absolutely veto this suggestion for general adoption. Specific cases, fine. I know of one existing bug in httpd that I would consider a showstopper, if I were RM, due to the way APR handles time. In order to fix it, I am going to need to reinstate handling of time in seconds, even if that means abandoning APR's routines. Please share, I'm certain a few more pairs of eyes could prove useful. ++1 on distinguishing apr_time_t to be more meaningful, though!
Re: apr_time_t -- apr_time_usec_t
On Mon, Jun 10, 2002 at 03:57:29PM -0700, Justin Erenkrantz wrote: How about using the value we already have: typedef apr_int32_t apr_short_interval_time_t; Unfortunately, that type still has units of milliseconds. Seems like Roy needs an apr_seconds_t, and apr_short_interval_time_t should be renamed apr_milliseconds_t. -aaron
Re: apr_time_t -- apr_time_usec_t
On Mon, Jun 10, 2002 at 04:14:14PM -0700, Aaron Bannert wrote: Unfortunately, that type still has units of milliseconds. Seems like Roy needs an apr_seconds_t, and apr_short_interval_time_t should be renamed apr_milliseconds_t. er, s/milliseconds/microseconds/ -aaron
Re: apr_time_t -- apr_time_usec_t
On Monday, June 10, 2002, at 04:11 PM, William A. Rowe, Jr. wrote: At 05:04 PM 6/10/2002, you wrote: I am tired of seeing this stupid change to the semantics of time_t under Unix continue to cause bugs in every project that uses APR. I must have missed that discussion traveling. Pointers please? Just do a search for 100 or USEC in the cvs archives. I've seen at least two dozen separate bug fixes applied to APR and httpd that were directly caused by people changing time_t to apr_time_t and expecting it to work. The latest one floated by this weekend. Bad interfaces are evidenced by repetition of the same bugs over time. apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* No. I will veto the change from time_t to apr_time_t due to misleading changes in semantics. I am going back to the state wherein httpd was robust. Since my change will both reduce bugs and improve performance, without sacrificing any known features, there is sufficient justification for making it in httpd. You can debate how this should effect APR all you like. I'd prefer a solution that is general to APR. I don't know of any APR client apps that make use of microseconds other than flood and ab. time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Fine, that is one solution. As far as adopting apr_time_sec_t throughout, you may be looking forward to your retirement party before a signed 32 bit apr_time_sec_t blows chunks. Having coded against Y2K since 1989, I'd absolutely veto this suggestion for general adoption. Specific cases, fine. time_t is not limited to 32 bits. I never mentioned 32 bits. You must be referring to someone else. Doing 64 bit divides and multiplies are bad for performance, but that is done because we are storing microseconds even though we never use microseconds. Eliminate the microseconds, or move it to a separate field, but keep the 64 bits. I personally think it a waste of time (no pun intended) to worry about a 32bit time_t when time_t is only 32 bits on machines that only have a 32bit interface to their time operations, and hence our clock will still roll-over regardless of how many extra bits we use to store our copy of time_t. I will discuss httpd changes on [EMAIL PROTECTED] -- this thread was just to get a little frustration off my chest regarding APR design decisions that are made based on the theory that they might some day be needed and then never changed (in spite of several requests on my part) based on the theory that they might break some mythical user of APR. It's time to get the clue stick out of the closet. From now on, apr_time_t is considered harmful. Roy
Re: apr_time_t -- apr_time_usec_t
William A. Rowe, Jr. wrote: apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Agreed. But, IMO, it *is* documented that apr_time_t is microsecond resolution. If people make assumptions then, well, that's bad, but not really a showstopper as far as I'm concerned. Now the nastyness of 64bit mult/division when we (always) need second resolution is another. Sure would be nice if it was an exact power of 2 :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: apr_time_t -- apr_time_usec_t
At 09:15 PM 6/10/2002, you wrote: William A. Rowe, Jr. wrote: apr_time_t must be in seconds. If folks want APR to keep time in microseconds, then they had bloody well change the type name accordingly. apr_time_t must nothing :-) Let's discuss *should(s)* time_t is seconds. I love the idea of apr_time_usec_t and apr_time_sec_t names rather that something as ambigous as apr_time_t (which is misleading, I agree.) Agreed. But, IMO, it *is* documented that apr_time_t is microsecond resolution. If people make assumptions then, well, that's bad, but not really a showstopper as far as I'm concerned. Now the nastyness of 64bit mult/division when we (always) need second resolution is another. Sure would be nice if it was an exact power of 2 :) It can be. busec [binary microseconds] has a resolution of 1 / 2^20, leaving 2^41-1 for minutes [and providing for negative time.] That yields 69000-some years forwards or backwards from Jan 1 1970. busec translation would be pretty trivial... time 20 == seconds time (2^20 - 1) == busec (binary microseconds) usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec The nice thing is that you can do all the addition, subtraction and multiplication you want and come back to seconds/usec values anytime you like. You don't need to perform separate math operations with carry on two separate fields, which is the significant fault to the sec, usec structure case. The question is, how often do you need to convert usec between decimal and binary? Really only to implode or explode the time, either in display/gregorian format or sec/usec structures. Just some boolean food for thought. Bill
Re: apr_time_t -- apr_time_usec_t
At 09:31 PM 6/10/2002, I wrote: usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec whoops, s/1048575/1048576/
Re: apr_time_t -- apr_time_usec_t
William A. Rowe, Jr. wrote: At 09:31 PM 6/10/2002, I wrote: usec * 1048575 / 100 = busec bsec * 100 / 1048575 = usec whoops, s/1048575/1048576/ That's a major improvement. I was going to complain about the need for both a 64-bit multiplication and a 64-bit division to do the conversion, but 1048576 only requires a shift. In general, I like this design: * It combines most of the benefits of the struct apr_timeval approach with most of the benefits of the current seconds*100+microseconds scalar representation. * It leaves 44 bits of a 64-bit representation free to hold the time_t, so it won't overflow for a few hundred thousand years. * And the transition is easy: just change APR_USEC_PER_SEC to 1048576 and tell all users of APR to recompile. +1 for the binary microseconds implementation -0 for continuing to call it apr_time_t --Brian
Re: [APR PATCH] Bootstrapping problem on Mac OS X
On Mon, Jun 10, 2002 at 01:44:05AM -0700, David Mankin wrote:
...
I configured and built svn with the --enable-maintainer-mode and
--disable-shared.
...
However, I think there might also be a bug (or several) in Subversion.
I compiled with --disable-shared, and yet subversion is still trying to
load a dynamic library. I'm trying to do an HTTP checkout, and yet it's
trying to load a dynamic library for RA local. Both of these seem like
they might be problems.
Not a problem, actually.
The RA layer knows that ra_dav was linked in, and that ra_local is *not*
linked in. Thus, it attempts to load it dynamically since the DSO facilities
are available in APR.
The --disable-shared switch controls whether shared libraries are *built*.
It doesn't control whether SVN will attempt to find RA modules that were not
linked into the app.
(And even with my patch to APR, I still get a
warning, about not being able to load libsvn_ra_local.so, every time I
do anything with the client. Maybe my patch should throw away the error
instead of printing it?)
Hmm? What is printing that warning? I don't see any code in ra_loader.c that
would do that.
[A fourth thing that may be an issue: when I modified dso.c and then
re-ran make from the top-level svn makefile, it didn't re-link the svn
binary, even though the apr.la (?) library had changed. I had to touch
clients/cmdline/main.c in order to force make to rebuild svn.]
Hmm. That does seem to be a problem. Could you file an issue about that?
...
+/*
+ * Under Darwin, we need to have a linkEdit error handler, or else if
+ * NSAddLibrary() fails, it will exit the whole program. This function
prints
+ * the same message that the OS would, but does not exit the program.
+ */
+#if defined(DSO_USE_DYLD)
+APR_DECLARE(void) apr_dso_load_linkEdit_errorhander(NSLinkEditErrors
errorClass,
+int errorNumber,
+const char *filename,
+const char *errorString)
+{
+if (errorString != NULL) {
+fprintf(stderr, %s, errorString);
+} else {
+fprintf(stderr, LinkEdit error! errorno = %d\n, errorNumber);
+}
The APR library is not allowed to use stderr. The only thing you can do is
return errors.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [APR PATCH] Bootstrapping problem on Mac OS X
On Mon, Jun 10, 2002 at 05:00:23PM +0100, Pier Fumagalli wrote: Hmm... I basically rewrote that part, so, I believe it's my fault... Yes, you're right, without the default error handlers, the baby exits (but this is not a problem in Apache 2.0 as DSO modules are required to be there at startup, but your point is right, if we don't have to terminate the process, we'd better not to... Correct. Exiting is up to the application, not APR. Now, the question is _why_ that thing is called and why libsvn_ra_local.so does not exist (if shared is disabled, it should be compiled in, right?)... Nope. ra_local was probably not compiled and linked in because the Berkeley DB libraries are not available, so the FS was not compiled, so ra_local was skipped because it had no FS to link to. Missing ra_local is a standard remote client only configuration. Cheers, -g -- Greg Stein, http://www.lyra.org/
Bugzilla default owner...
Anyone has a clue on why the default owner for APR bugs is [EMAIL PROTECTED] and not [EMAIL PROTECTED] Pier
Re: [APR PATCH] Bootstrapping problem on Mac OS X
On Mon, 10 Jun 2002, Greg Stein wrote:
(And even with my patch to APR, I still get a
warning, about not being able to load libsvn_ra_local.so, every time I
do anything with the client. Maybe my patch should throw away the error
instead of printing it?)
Hmm? What is printing that warning? I don't see any code in ra_loader.c that
would do that.
It was my patch printing that warning (instead of Darwin that used to
print the warning before exiting). See below...
...
+/*
+ * Under Darwin, we need to have a linkEdit error handler, or else if
+ * NSAddLibrary() fails, it will exit the whole program. This function
prints
+ * the same message that the OS would, but does not exit the program.
+ */
+#if defined(DSO_USE_DYLD)
+APR_DECLARE(void) apr_dso_load_linkEdit_errorhander(NSLinkEditErrors
errorClass,
+int errorNumber,
+const char *filename,
+const char
*errorString)
+{
+if (errorString != NULL) {
+fprintf(stderr, %s, errorString);
+} else {
+fprintf(stderr, LinkEdit error! errorno = %d\n, errorNumber);
+}
The APR library is not allowed to use stderr. The only thing you can do is
return errors.
Thanks, Greg. See, I told ya I don't know much about APR. I printed the
error message just like Darwin does. (Actually it uses fd 2, instead of
stderr, but I doubt that APR should do that either.) I'll take the
printing out of my patch.
I don't know how to get the message back to the client, though. The
message is passed to the error-handler callback, and not returned on the
NSAddLibrary() call. Maybe the callback should store the pointer to the
message in some static variable that the NSAddLibrary can return as the
error? The thread-safety-conscious Java programmer in me doesn't like
that very much, but maybe it would be okay? Pseudo code:
static char* last_error_message = NULL;
void apr_dso_load_linkedit_error_handler(const char* msg, ...)
{
last_error_message = msg;
}
apr_status_t apr_dso_load(...)
{
char* err_msg = NULL;
...
else if (NSAddLibrary(path) == TRUE) {
...
} else {
err_msg = last_error_message ? last_error_message :
cannot create object file image or add library;
last_error_message = NULL;
}
...
}
This doesn't seem safe, but I can't think of another way to get the OS
error message out. (But it's been a *long* time since I've coded in C,
maybe I'm missing something.) It'd be a shame to just lose the OS error
with its useful information (file not found, e.g.) and replace it with
cannot add library.
Ideas suggestions welcome.
-David Mankin
Re: Bugzilla default owner...
On Tue, Jun 11, 2002 at 01:18:09PM +0100, Pier Fumagalli wrote: Anyone has a clue on why the default owner for APR bugs is [EMAIL PROTECTED] and not [EMAIL PROTECTED] Because there is no [EMAIL PROTECTED] list. =) -- justin
Re: repository conversion on windows fails / binary file
On Tue, Jun 11, 2002 at 10:10:22PM +0200, Branko Cibej wrote: ... The solution is to reopen the streams in binary mode, obviously. I wonder how you do that in APR. Pass APR_BINARY to the apr_file_open() function. Not a problem. Of course, it appears that apr/file_io/win32/open.c doesn't even look for that flag :-( Hmm. And reopening stdout/stdin in binary mode... oof. No fricking clue. Cheers, -g -- Greg Stein, http://www.lyra.org/
RE: repository conversion on windows fails / binary file
From: Greg Stein [mailto:[EMAIL PROTECTED] On Tue, Jun 11, 2002 at 10:10:22PM +0200, Branko Cibej wrote: ... The solution is to reopen the streams in binary mode, obviously. I wonder how you do that in APR. Pass APR_BINARY to the apr_file_open() function. Not a problem. Of course, it appears that apr/file_io/win32/open.c doesn't even look for that flag :-( That is a pretty major bug. :-( Hmm. And reopening stdout/stdin in binary mode... oof. No fricking clue. We don't do that currently. You really only have two options as things stand today. 1) Close the file and re-open. 2) Implement apr_file_reopen (which devolves to option #1 in the worst case scenario). Ryan
Re: repository conversion on windows fails / binary file
Ryan Bloom wrote:
From: Greg Stein [mailto:[EMAIL PROTECTED]
On Tue, Jun 11, 2002 at 10:10:22PM +0200, Branko Cibej wrote:
...
The solution is to reopen the streams in binary mode, obviously. I
wonder how you do that in APR.
Pass APR_BINARY to the apr_file_open() function. Not a problem.
Of course, it appears that apr/file_io/win32/open.c doesn't even look
for
that flag :-(
That is a pretty major bug. :-(
Hmm. And reopening stdout/stdin in binary mode... oof. No fricking
clue.
We don't do that currently. You really only have two options as things
stand today. 1) Close the file and re-open. 2) Implement
apr_file_reopen (which devolves to option #1 in the worst case
scenario).
Ryan
/me has a more evil solution. Use apr_file_open_std*, as that bypasses
the CRT and uses the OS's file handles which don't know or care about
translation. The attached patch works on Win32.
Even if it's not the Totally Right Thing To Do, I vote we put it on
trunk/ and branches/fs-convert-2092/ and roll new tarballs, or at least
Win32 binaries.
--
Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Index: ./main.c
===
--- ./main.c
+++ ./main.c2002-06-12 00:36:10.0 +0200
@@ -17,6 +17,7 @@
*/
+#include apr_file_io.h
#include svnadmin.h
typedef enum svnadmin_cmd_t
@@ -467,6 +468,8 @@
case svnadmin_cmd_dump:
{
svn_stream_t *stdout_stream, *stderr_stream;
+apr_file_t *stdout_file, *stderr_file;
+apr_status_t apr_err;
svn_revnum_t
lower = SVN_INVALID_REVNUM,
upper = SVN_INVALID_REVNUM;
@@ -493,29 +496,51 @@
/* Run the dump to STDOUT. Let the user redirect output into
a file if they want. :-) Progress feedback goes to stderr. */
-stdout_stream = svn_stream_from_stdio (stdout, pool);
-stderr_stream = svn_stream_from_stdio (stderr, pool);
+apr_err = apr_file_open_stdout (stdout_file, pool);
+if (apr_err)
+ INT_ERR (svn_error_createf (apr_err, 0, NULL, pool,
+ %s: error opening stdout, argv[0]));
+stdout_stream = svn_stream_from_aprfile (stdout_file, pool);
+
+apr_err = apr_file_open_stderr (stderr_file, pool);
+if (apr_err)
+ INT_ERR (svn_error_createf (apr_err, 0, NULL, pool,
+ %s: error opening stderr, argv[0]));
+stderr_stream = svn_stream_from_aprfile (stderr_file, pool);
INT_ERR (svn_repos_dump_fs (repos, stdout_stream, stderr_stream,
lower, upper, pool));
-fflush(stdout);
+apr_err = apr_file_flush(stdout_file);
+if (apr_err)
+ INT_ERR (svn_error_createf (apr_err, 0, NULL, pool,
+ %s: error flushing stdout, argv[0]));
}
break;
case svnadmin_cmd_load:
{
svn_stream_t *stdin_stream, *stdout_stream;
+apr_file_t *stdin_file, *stdout_file;
+apr_status_t apr_err;
INT_ERR (svn_repos_open (repos, path, pool));
fs = svn_repos_fs (repos);
/* Read the stream from STDIN. Users can redirect a file. */
-stdin_stream = svn_stream_from_stdio (stdin, pool);
-
+apr_err = apr_file_open_stdin (stdin_file, pool);
+if (apr_err)
+ INT_ERR (svn_error_createf (apr_err, 0, NULL, pool,
+ %s: error opening stdin, argv[0]));
+stdin_stream = svn_stream_from_aprfile (stdin_file, pool);
+
/* Have the parser dump feedback to STDOUT. */
-stdout_stream = svn_stream_from_stdio (stdout, pool);
-
+apr_err = apr_file_open_stdout (stdout_file, pool);
+if (apr_err)
+ INT_ERR (svn_error_createf (apr_err, 0, NULL, pool,
+ %s: error opening stdout, argv[0]));
+stdout_stream = svn_stream_from_aprfile (stdout_file, pool);
+
INT_ERR (svn_repos_load_fs (repos, stdin_stream, stdout_stream, pool));
}
break;
