Re: [PATCH] #7 OS400/EBCDIC Port: Make svn_io_copy_file() CCSID insensitive.

Julian Foad <julianfoad@btopenworld.com> wrote on 03/07/2006 09:56:00 AM:

Hi Julian,

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
we
> 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
source
> 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
might
> do translations on text files. However, it's broken on non-Unix
systems.
>
> There are two possible causes of unwanted modification during a copy.
>
> (1) If the source and destination are opened in different modes (text
vs.
> binary) or, on OS400, have different CCSIDs. This should not occur
because
> this is a complete COPY operation in which the destination file is
completely
> under the function's control. The destination should be opened in the
same
> mode, and have the same CCSID as the source. If that's not easy to do
with
> APR, that's a bug in APR or needs to be done with lower-level system
calls.

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.
This
> should be avoided by opening the files in "binary" mode so that
byte-for-byte
> 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
fix
> 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
assurance
> 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
either
> 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
will
   * 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
source
> > + * file from its CCSID to the CCSID of the desination file. This may
>
> ("desination" -> "destination")

Fixed.
 
> > + * corrupt the destination file's contents if the files' CCSIDs
differ from
> > + * 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
apr_file_transfer_contents in
> > + * srclib/apr/file_io/unix/copy.c of version 2.0.54 of the Apache
HTTP
> > + * Server (http://httpd.apache.org/) excepting that APR_LARGEFILE is
not
> > + * 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,
pool);
> > +#else
> > + /* Do a CCSID blind file copy on OS400. */
>
> (No need for that comment. No space before paren in next line(s).)

Gone & gone.

Paul B.

[[[
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.
]]]

_____________________________________________________________________________
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