You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pa...@softlanding.com> on 2006/02/13 21:10:53 UTC
[PATCH] #3 Update on port to OS400/EBCDIC
Hello All,
This is the third in a series of patch submissions that will allow
subversion to run on the IBM iSeries under OS400 V5R4.
See: http://svn.haxx.se/dev/archive-2006-02/0519.shtml
Appreciate it if anyone has some time to review this.
Thanks,
Paul B.
[[[
OS400/EBCDIC Port: apr_poll() signature differences.
This is the third of several patches to allow Subversion to run on IBM's
OS400 V5R4.
IBM's implementation of apr_poll() requires a pool argument. We have also
experienced unpredictable failures with apr_poll() when the nsds parameter
is uninitialized.
* subversion/libsvn_ra_svn/marshal.c
(svn_ra_svn__input_waiting):
* subversion/libsvn_subr/prompt.c
(wait_for_input):
Initialize nsds parameter and supply pool argument for
apr_poll() calls.
]]]
Index: subversion/libsvn_ra_svn/marshal.c
===================================================================
--- subversion/libsvn_ra_svn/marshal.c (revision 18448)
+++ subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -121,7 +121,15 @@
}
pfd.p = pool;
pfd.reqevents = APR_POLLIN;
+#ifndef AS400
return ((apr_poll(&pfd, 1, &n, 0) == APR_SUCCESS) && n);
+#else
+ /* IBM's apr_poll() implmentation behaves badly with some large values
+ * of n (apr_palloc fails) so we initialize it. */
+ n = 0;
+ /* OS400 requires a pool argument for apr_poll(). */
+ return ((apr_poll(&pfd, 1, &n, 0, pool) == APR_SUCCESS) && n);
+#endif
}
/* --- WRITE BUFFER MANAGEMENT --- */
Index: subversion/libsvn_subr/prompt.c
===================================================================
--- subversion/libsvn_subr/prompt.c (revision 18448)
+++ subversion/libsvn_subr/prompt.c (working copy)
@@ -59,7 +59,15 @@
pollset.p = pool;
pollset.reqevents = APR_POLLIN;
+#ifndef AS400
srv = apr_poll(&pollset, 1, &n, -1);
+#else
+ /* IBM's apr_poll() implmentation behaves badly with some large values
+ * of n (apr_palloc fails) so we initialize it. */
+ n = 0;
+ /* OS400 requires a pool argument for apr_poll(). */
+ srv = apr_poll(&pollset, 1, &n, -1, pool);
+#endif
if (n == 1 && pollset.rtnevents & APR_POLLIN)
return APR_SUCCESS;
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: APR APIs and their doc strings [was: [PATCH] #3 Update on port to OS400/EBCDIC]
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/13/06, Julian Foad <ju...@btopenworld.com> wrote:
> Garrett Rooney wrote:
> > On 2/13/06, Julian Foad <ju...@btopenworld.com> wrote:
> >
> >>It would make sense for mainstream APR to revise this API, adding the pool
> >>parameter. Do you know if IBM has proposed that to the APR developers (or are
> >>likely to)? Not that we will be able to rely on it for a long time yet, but it
> >>should happen anyway.
> >
> > No, they have not proposed it, and even if we wanted to change it we
> > can't do so until APR 2.0.
>
> Is there no API revision mechanism, like in Subversion we would introduce
> apr_poll2() and deprecate apr_poll() ?
Well, that could certainly be done, but historically it doesn't seem
to happen very often.
> >>(If anyone wants a (miserable) laugh, take a look at the doc string for
> >>apr_wait_for_io_or_timeout():
> >
> > Ha. That is amusing. Will fix.
>
> Oh, thanks, Garrett; I was just starting to subscribe to the APR mailing list
> in order to post a note about this and a couple of other lesser doc string
> improvements that I've noted recently. If you're in the mood for it, this is
> what I was going to send:
I'll look at getting these dealt with, thanks.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
APR APIs and their doc strings [was: [PATCH] #3 Update on port to
OS400/EBCDIC]
Posted by Julian Foad <ju...@btopenworld.com>.
Garrett Rooney wrote:
> On 2/13/06, Julian Foad <ju...@btopenworld.com> wrote:
>
>>It would make sense for mainstream APR to revise this API, adding the pool
>>parameter. Do you know if IBM has proposed that to the APR developers (or are
>>likely to)? Not that we will be able to rely on it for a long time yet, but it
>>should happen anyway.
>
> No, they have not proposed it, and even if we wanted to change it we
> can't do so until APR 2.0.
Is there no API revision mechanism, like in Subversion we would introduce
apr_poll2() and deprecate apr_poll() ?
>>(If anyone wants a (miserable) laugh, take a look at the doc string for
>>apr_wait_for_io_or_timeout():
>
> Ha. That is amusing. Will fix.
Oh, thanks, Garrett; I was just starting to subscribe to the APR mailing list
in order to post a note about this and a couple of other lesser doc string
improvements that I've noted recently. If you're in the mood for it, this is
what I was going to send:
Index: apr/include/apr_getopt.h
===================================================================
--- apr/include/apr_getopt.h (revision 128444)
+++ apr/include/apr_getopt.h (working copy)
@@ -34,7 +34,8 @@
*/
/**
- * defintion of a error function
+ * An error function.
+ * ### What are the parameters, especially "arg"?
*/
typedef void (apr_getopt_err_fn_t)(void *arg, const char *err, ...);
@@ -94,8 +95,8 @@
* @param cont The pool to operate on
* @param argc The number of arguments to parse
* @param argv The array of arguments to parse
- * @remark Arguments 2 and 3 are most commonly argc and argv from main(argc, argv)
- * The errfn is initialized to fprintf(stderr... but may be overridden.
+ * @remark Arguments 3 and 4 are most commonly argc and argv from main(argc, argv)
+ * (*os)->errfn is initialized to fprintf(stderr... but may be overridden.
*/
APR_DECLARE(apr_status_t) apr_getopt_init(apr_getopt_t **os, apr_pool_t *cont,
int argc, const char * const *argv);
Index: apr/include/apr_poll.h
===================================================================
--- apr/include/apr_poll.h (revision 128444)
+++ apr/include/apr_poll.h (working copy)
@@ -151,7 +151,7 @@
* Poll the sockets in the poll structure
* @param aprset The poll structure we will be using.
* @param numsock The number of sockets we are polling
- * @param nsds The number of sockets signalled.
+ * @param nsds The number of sockets signalled (output parameter)
* @param timeout The amount of time in microseconds to wait. This is
* a maximum, not a minimum. If a socket is signalled, we
* will wake up before this time. A negative number means
Index: apr/include/apr_support.h
===================================================================
--- apr/include/apr_support.h (revision 128444)
+++ apr/include/apr_support.h (working copy)
@@ -38,7 +38,8 @@
/**
* Wait for IO to occur or timeout.
*
- * Uses POOL for temporary allocations.
+ * ### What IO? What timeout? On return, does it indicate which happened?
+ * ### What are the arguments? Shouldn't it take a pool argument?
*/
apr_status_t apr_wait_for_io_or_timeout(apr_file_t *f, apr_socket_t *s,
int for_read);
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #3 Update on port to OS400/EBCDIC
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/13/06, Julian Foad <ju...@btopenworld.com> wrote:
> It would make sense for mainstream APR to revise this API, adding the pool
> parameter. Do you know if IBM has proposed that to the APR developers (or are
> likely to)? Not that we will be able to rely on it for a long time yet, but it
> should happen anyway.
No, they have not proposed it, and even if we wanted to change it we
can't do so until APR 2.0.
> (If anyone wants a (miserable) laugh, take a look at the doc string for
> apr_wait_for_io_or_timeout():
>
> /**
> * Wait for IO to occur or timeout.
> *
> * Uses POOL for temporary allocations.
> */
> apr_status_t apr_wait_for_io_or_timeout(apr_file_t *f, apr_socket_t *s,
> int for_read);
>
> What a pile of poo! Somebody's not been reading what they're writing.
>
> OK, I suppose I ought not to ridicule it here but report it to the proper
> authority.)
Ha. That is amusing. Will fix.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #3 Update on port to OS400/EBCDIC
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> Julian Foad <ju...@btopenworld.com> wrote on 02/13/2006 05:31:50 PM:
>>
>>That sounds like a trivial bug. Will IBM soon release a version of APR with
>>this bug fixed?
>
> Damn, I should assume nothing. While IBM didn't explicitly tell us they
> fixed this problem, I just rechecked and one of their recent V5R4 PTFs
> (Program Temporary Fix) for OS400 fixed it. So this initialization is N/A
> now and the patch has a bit less "yuck".
Excellent news.
>>Well, honestly, yuck, but as it's only twice, and doesn't touch any of our
>>APIs, I could accept this patch as it is.
>
> Can I take this as a +1? :-)
Yes! Please commit it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] #3 Update on port to OS400/EBCDIC
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 02/13/2006 05:31:50 PM:
> Paul Burba wrote:
> > Index: subversion/libsvn_ra_svn/marshal.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/marshal.c (revision 18448)
> > +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> > @@ -121,7 +121,15 @@
> > }
> > pfd.p = pool;
> > pfd.reqevents = APR_POLLIN;
> > +#ifndef AS400
> > return ((apr_poll(&pfd, 1, &n, 0) == APR_SUCCESS) && n);
> > +#else
> > + /* IBM's apr_poll() implmentation behaves badly with some large
values
>
> Typo: "implementation" (in both files).
>
> > + * of n (apr_palloc fails) so we initialize it. */
> > + n = 0;
>
> That sounds like a trivial bug. Will IBM soon release a version of APR
with
> this bug fixed?
Damn, I should assume nothing. While IBM didn't explicitly tell us they
fixed this problem, I just rechecked and one of their recent V5R4 PTFs
(Program Temporary Fix) for OS400 fixed it. So this initialization is N/A
now and the patch has a bit less "yuck".
> > + /* OS400 requires a pool argument for apr_poll(). */
> > + return ((apr_poll(&pfd, 1, &n, 0, pool) == APR_SUCCESS) && n);
> > +#endif
>
> It would make sense for mainstream APR to revise this API, adding the
pool
> parameter. Do you know if IBM has proposed that to the APR
> developers (or are
> likely to)?
I think it's safe to say that IBM won't address it.
> > pollset.p = pool;
> > pollset.reqevents = APR_POLLIN;
> >
> > +#ifndef AS400
> > srv = apr_poll(&pollset, 1, &n, -1);
> > +#else
> > + /* IBM's apr_poll() implmentation behaves badly with some large
values
> > + * of n (apr_palloc fails) so we initialize it. */
> > + n = 0;
> > + /* OS400 requires a pool argument for apr_poll(). */
> > + srv = apr_poll(&pollset, 1, &n, -1, pool);
> > +#endif
> >
>
> Well, honestly, yuck, but as it's only twice, and doesn't touch any of
our
> APIs, I could accept this patch as it is.
Can I take this as a +1? :-)
[[[
OS400/EBCDIC Port: apr_poll() signature differences.
This is the third of several patches to allow Subversion to run on IBM's
OS400 V5R4.
IBM's implementation of apr_poll() requires a pool argument.
Approved by: Julian Foad <ju...@btopenworld.com>
* subversion/libsvn_ra_svn/marshal.c
(svn_ra_svn__input_waiting):
* subversion/libsvn_subr/prompt.c
(wait_for_input):
Supply pool argument for apr_poll() calls on OS400.
]]]
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
Re: [PATCH] #3 Update on port to OS400/EBCDIC
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
>
> [[[
> OS400/EBCDIC Port: apr_poll() signature differences.
>
> This is the third of several patches to allow Subversion to run on IBM's
> OS400 V5R4.
>
> IBM's implementation of apr_poll() requires a pool argument. We have also
> experienced unpredictable failures with apr_poll() when the nsds parameter
> is uninitialized.
>
> * subversion/libsvn_ra_svn/marshal.c
> (svn_ra_svn__input_waiting):
> * subversion/libsvn_subr/prompt.c
> (wait_for_input):
> Initialize nsds parameter and supply pool argument for
> apr_poll() calls.
> ]]]
>
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 18448)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -121,7 +121,15 @@
> }
> pfd.p = pool;
> pfd.reqevents = APR_POLLIN;
> +#ifndef AS400
> return ((apr_poll(&pfd, 1, &n, 0) == APR_SUCCESS) && n);
> +#else
> + /* IBM's apr_poll() implmentation behaves badly with some large values
Typo: "implementation" (in both files).
> + * of n (apr_palloc fails) so we initialize it. */
> + n = 0;
That sounds like a trivial bug. Will IBM soon release a version of APR with
this bug fixed? In that case we wouldn't need this work-around. The
doc-string of apr_poll ought to be improved to say that this is an output
parameter; it's a bit vague presently.
> + /* OS400 requires a pool argument for apr_poll(). */
> + return ((apr_poll(&pfd, 1, &n, 0, pool) == APR_SUCCESS) && n);
> +#endif
It would make sense for mainstream APR to revise this API, adding the pool
parameter. Do you know if IBM has proposed that to the APR developers (or are
likely to)? Not that we will be able to rely on it for a long time yet, but it
should happen anyway.
> }
>
> /* --- WRITE BUFFER MANAGEMENT --- */
> Index: subversion/libsvn_subr/prompt.c
> ===================================================================
> --- subversion/libsvn_subr/prompt.c (revision 18448)
> +++ subversion/libsvn_subr/prompt.c (working copy)
> @@ -59,7 +59,15 @@
The doc string of this function (wait_for_input) is pretty unintelligible, but
it's not for you to fix it.
(If anyone wants a (miserable) laugh, take a look at the doc string for
apr_wait_for_io_or_timeout():
/**
* Wait for IO to occur or timeout.
*
* Uses POOL for temporary allocations.
*/
apr_status_t apr_wait_for_io_or_timeout(apr_file_t *f, apr_socket_t *s,
int for_read);
What a pile of poo! Somebody's not been reading what they're writing.
OK, I suppose I ought not to ridicule it here but report it to the proper
authority.)
> pollset.p = pool;
> pollset.reqevents = APR_POLLIN;
>
> +#ifndef AS400
> srv = apr_poll(&pollset, 1, &n, -1);
> +#else
> + /* IBM's apr_poll() implmentation behaves badly with some large values
> + * of n (apr_palloc fails) so we initialize it. */
> + n = 0;
> + /* OS400 requires a pool argument for apr_poll(). */
> + srv = apr_poll(&pollset, 1, &n, -1, pool);
> +#endif
>
Well, honestly, yuck, but as it's only twice, and doesn't touch any of our
APIs, I could accept this patch as it is.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org