Julian Foad <email@example.com> wrote on 03/07/2006 09:56:00 AM:
Thanks for reviewing this. I know this OS400 stuff can be a haze...
> Paul Burba wrote:
> > [[[
> > OS400/EBCDIC Port: Make svn_io_copy_file CCSID insensitive.
> > This is one of several patches to allow Subversion to run on IBM's
> > OS400 V5R4. It makes svn_io_copy_file() insensitive to the CCSIDs of
> > the source and destination files, thereby preventing potential OS400
> > conversion of the files' contents.
> > * subversion/libsvn_subr/io.c
> > (os400_file_copy): New function.
> > (svn_io_copy_file): Replace call to apr_file_copy() with new
> > function.
> > ]]]
> What do we want svn_io_copy_file() to do, in high-level terms? It seems
> want it to make an exact copy of a file, at least an exact copy of the
> contents, but perhaps not the meta-data. But it's silly, isn't it,
> to preserve
> the contents while not preserving the CCSID, because then the new
> file's CCSID
> would be more likely to be wrong, so this function should copy the
> file's CCSID too.
> I believe the apr_file_copy() was intended to do a byte-for-byte
> copy and thus
> correctly copy any file, text or binary, even on systems that otherwise
> do translations on text files. However, it's broken on non-Unix
> There are two possible causes of unwanted modification during a copy.
> (1) If the source and destination are opened in different modes (text
> binary) or, on OS400, have different CCSIDs. This should not occur
> this is a complete COPY operation in which the destination file is
> under the function's control. The destination should be opened in the
> mode, and have the same CCSID as the source. If that's not easy to do
> APR, that's a bug in APR or needs to be done with lower-level system
FWIW, IBM APR does not provide any way to obtain, specify, or otherwise
manipulate a file's CCSID. It can be done with lower-level system calls;
IBM provides a rather "cumbersome" (as in requiring a new 30+ line
function) API (Qp0lSetAttr) to change the CCSID of an IFS file.
> (2) If translating reads and writes are used, and the translation is not
> exactly reversible, so read + write causes an unintended translation.
> should be avoided by opening the files in "binary" mode so that
> reads and writes are performed. That's the bug that Max Bowsher
> found for the
> CygWin port of APR: it needs to read and write in binary mode. The same
> should make the reads and writes insensitive to CCSIDs on OS400.
> This should all be done within APR, of course. I can accept this
> patch on the
> basis that it won't get into APR soon enough, but I would like some
> that one of you will make an effort to get it into APR.
We'll push IBM to make this change, as we have with others, in their port
of APR as soon as it's in open source APR. We'll also suggest to them
that they may simply want to fix the problem in their port of APR,
regardless of what happens with open source APR, but this is less likely.
Regardless of what IBM elects to do, please understand that this is an
informal process; there's no mailing list for IBM APR or anything like
that. We simply don't have much power to make them do anything they don't
want to do.
> To complete this patch, svn_io_copy_file() needs to say in its doc-
> string that
> it does a "byte-for-byte" copy, as it is no longer just a wrapper around
> apr_file_copy(). (Eventually, I hope apr_file_copy() will be fixed and
> documented to do the same.)
Do we really want to do that? It becomes true for OS400 yes, and it's
been implicitly true for *nix, but it still isn't true for CygWin, no? Am
I being too much of a stickler here...or did you mean for the doc-string
to indicate that there are platform dependent differences?
> > +#ifdef AS400
> > +/* CCSID insensitive replacement for apr_file_copy() on OS400.
> It's not simply a replacement, as it has an extra argument. Please
> remove or document the extra argument.
I removed that argument, it was not necessary. I should have read my own
comment, which explained why:
* apr_file_copy() does not require the destination file to exist and
* overwrite it if it does. Since this is a replacement for
* apr_file_copy() we enforce similar behavior.
> > + *
> > + * (See comments for file_open() for more info on CCSIDs.)
> > + *
> > + * On OS400 apr_file_copy() attempts to convert the contents of the
> > + * file from its CCSID to the CCSID of the desination file. This may
> ("desination" -> "destination")
> > + * corrupt the destination file's contents if the files' CCSIDs
> > + * each other and/or the system CCSID.
> > + *
> > + * This new function prevents this by forcing a binary copy. It is
> > + * stripped down copy of the private function
> > + * srclib/apr/file_io/unix/copy.c of version 2.0.54 of the Apache
> > + * Server (http://httpd.apache.org/) excepting that APR_LARGEFILE is
> > + * used, from_path is always opened with APR_BINARY, and
> > + * APR_FILE_SOURCE_PERMS is not supported.
> > + */
> > +static apr_status_t
> > +os400_file_copy(const char *from_path,
> > + const char *to_path,
> > + apr_int32_t flags,
> > + apr_fileperms_t perms,
> > + apr_pool_t *pool)
> > +#ifndef AS400
> > apr_err = apr_file_copy(src_apr, dst_tmp_apr, APR_OS_DEFAULT,
> > +#else
> > + /* Do a CCSID blind file copy on OS400. */
> (No need for that comment. No space before paren in next line(s).)
Gone & gone.
OS400/EBCDIC Port: Make svn_io_copy_file CCSID insensitive.
This is one of several patches to allow Subversion to run on IBM's
OS400 V5R4. It makes svn_io_copy_file() insensitive to the CCSIDs of
the source and destination files, thereby preventing potential OS400
conversion of the files' contents.
(os400_file_copy): New function.
(svn_io_copy_file): Replace call to apr_file_copy() with new
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
Received on Wed Mar 8 21:44:53 2006
To unsubscribe, e-mail: firstname.lastname@example.org
For additional commands, e-mail: email@example.com