Re: [PATCH] #6 OS400/EBCDIC Port: Prevent OS conversion of file contents

Paul Burba <paulb@softlanding.com> writes:

> This patch blocks AS400 dependent code in two places like this:
>
> SVN_ERR(svn_path_cstring_from_utf8(&unique_name_apr, unique_name,
> pool));
> #ifdef AS400
> apr_err = os400_file_create_prep(unique_name_apr, &flag,
> APR_OS_DEFAULT, pool);
> if (!apr_err)
> #endif
> apr_err = apr_file_open(&file, unique_name_apr, flag | APR_BINARY,
> APR_OS_DEFAULT, pool);
>
> This obviously is improperly indented code, but is something like this
> ever acceptable considering the alternative would result in redundant code
> and generally be more intrusive?

It's not too bad IMO, however I would prefer an alternative solution,
see below.

> [[[
> Every file in OS400 is tagged with a CCSID which represents its
> character encoding. On OS400 V5R4 when apr_file_open() creates a
> file, its CCSID varies depending if the APR_BINARY flag is passed or
> not. If APR_BINARY is passed, the file is created with CCSID 37
> (EBCDIC), if not, it has CCSID 1209 (UTF-8).
>
> Since subversion creates files with either binary or UTF-8 content and
> all calls to apr_file_open() in subversion use APR_BINARY, these files
> are incorrectly tagged.
>
> Simply not using APR_BINARY on OS400 when opening a file isn't an
> option, because in this case the OS attempts to convert the file's
> contents from its CCSID to UTF-8 when reading the file and vice-versa
> when writing to it. This has obvious problems if the file contains
> binary data.

Are the above three paragraphs part of the log message? It looks like
the sort of information that should be in the code not the log, and it
looks like most of it is in the code.

> This patch ensures files *created* via svn_io_file_open() and
> svn_io_open_unique_file2() are tagged with a CCSID of 1208.
> ======================================================================
> * subversion/libsvn_subr/io.c
> (os400_file_create_prep): New function.
> (svn_io_open_unique_file2, svn_io_file_open): Call new function
> prior to calls to apr_file_open() that may create a new file.
> ]]]
>
>
> _____________________________________________________________________________
> Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
> _____________________________________________________________________________
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 18555)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -145,6 +145,46 @@
> }
>
>
> +#ifdef AS400
> +/* Helper function for apr_file_open() on OS400.
> + *
> + * When calling apr_file_open() with APR_BINARY and APR_CREATE on OS400
> + * the new file has an ebcdic CCSID (e.g. 37). But the files created by
> + * Subversion have either binary or utf-8 content, never ebcdic, so the
> + * files are incorrectly tagged. To force these files to the proper CCSID
> + * call this function prior to apr_file_open(), passing the filename and a
> + * reference to the flag apr_file_open() will use. The function creates an
> + * empty file with CCSID 1208, closes it, and removes APR_EXCL from flag.
> + */
> +apr_status_t

static

> +os400_file_create_prep(const char *fname,
> + apr_int32_t *flag,
> + apr_fileperms_t perm,
> + apr_pool_t *pool)
> +{
> + apr_file_t *f;
> + apr_status_t apr_err;
> +
> + /* If we are not trying to create a file there is nothing to do. */
> + if (~*flag & APR_CREATE)
> + return APR_SUCCESS;
> +
> + apr_err = apr_file_open (&f, fname, *flag & ~APR_BINARY, perm, pool);
> +
> + if (!apr_err)
> + apr_file_close(f);
> +
> + /* Whether or not APR_EXCL is set or not, we want to unset it before the
> + * call to apr_file_open() since this flag was already handled in the
> + * above call to open().
> + */
> + *flag &= ~APR_EXCL;
> +
> + return apr_err;
> +}
> +#endif
> +
> +
> svn_error_t *
> svn_io_check_resolved_path(const char *path,
> svn_node_kind_t *kind,
> @@ -264,6 +304,11 @@
> SVN_ERR(svn_path_cstring_from_utf8(&unique_name_apr, unique_name,
> pool));
>
> +#ifdef AS400
> + apr_err = os400_file_create_prep(unique_name_apr, &flag,
> + APR_OS_DEFAULT, pool);
> + if (!apr_err)
> +#endif
> apr_err = apr_file_open(&file, unique_name_apr, flag | APR_BINARY,
> APR_OS_DEFAULT, pool);
> @@ -2237,6 +2282,10 @@
> apr_status_t status;
>
> SVN_ERR(svn_path_cstring_from_utf8(&fname_apr, fname, pool));
> +#ifdef AS400
> + status = os400_file_create_prep(fname_apr, &flag, perm, pool);
> + if (!status)
> +#endif
> status = apr_file_open(new_file, fname_apr, flag | APR_BINARY, perm, pool);
>
> if (status)

I'd prefer not to have so many #ifdef AS400's in the code, how about
using something like

static apr_status_t file_open(...)
{
  ...
#ifdef AS400
   ...
   if (apr_err)
     return apr_err;
#endif
   apr_err = apr_file_open(...);
   return apr_err;
}

instead of os400_file_create_prep, then existing callers just change
from

      apr_err = apr_file_open(...);

to
      apr_err = file_open(...);

and the indentation problem goes away as well.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org