Erik Huelsmann wrote:
>
> * subversion/libsvn_subr/io.c:
> (svn_io_file_read,svn_io_file_read_full,
> svn_io_file_write,svn_io_file_write_full):
> New. Wrappers around apr_file_ calls returning svn_error_t's.
> (do_io_file_wrapper_cleanup):
> New. Implements cleanup procedure for the file wrappers above.
> (file_name_get): Ensure that NULL is set on error
> in *fname_utf8: a requirement for do_io_file_wrapper_cleanup.
> (svn_io_file_open): Return an error string formatted like the ones
> from do_io_file_wrapper_cleanup.
> (svn_io_file_close): Use do_io_file_wrapper_cleanup for
> error handling.
>
[...]
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 7872)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -973,13 +973,17 @@
> }
>
>
> -/* Get the name of FILE, or NULL if FILE is an unnamed stream. */
> +/* Get the name of FILE, or NULL if FILE is an unnamed stream.
> + On error *fname_utf8 is guaranteed to contain NULL.
> + */
> static svn_error_t *
> file_name_get (const char **fname_utf8, apr_file_t *file, apr_pool_t *pool)
> {
> apr_status_t apr_err;
> const char *fname;
>
> + *fname_utf8 = NULL;
> +
> apr_err = apr_file_name_get (&fname, file);
> if (apr_err)
> return svn_error_create
> @@ -987,10 +991,15 @@
> "failed to get file name from APR");
>
> if (fname)
> - SVN_ERR (svn_path_cstring_to_utf8 (fname_utf8, fname, pool));
> - else
> - *fname_utf8 = NULL;
> + {
> + svn_error_t *err;
>
> + err = svn_path_cstring_to_utf8 (&fname, fname, pool);
> + if (err)
> + return err;
> + }
> +
> + *fname_utf8 = fname;
> return SVN_NO_ERROR;
> }
It would be better to leave this function as it was, but in your new function "do_io_file_wrapper_cleanup", if file_name_get returns an error, don't try to use the resulting file name.
[...]
> +static svn_error_t *
> +do_io_file_wrapper_cleanup (apr_file_t *file, apr_status_t status,
> + const char *op, apr_pool_t *pool)
> +{
> + const char *name;
> + const char *errstr;
> + char errbuf[255];
> + svn_error_t *err;
> +
> + if (! status)
> + return SVN_NO_ERROR;
> +
> + svn_error_clear (file_name_get (&name, file, pool));
> + name = (name) ? apr_psprintf (pool, "file '%s'", name) : "stream";
Following on from what I said above: If file_name_get returns an error, don't try to use the resulting file name. Like when you call svn_utf_cstring_to_utf8 just below here, except that in this case there will be three possible results: valid file name, or NULL which you convert to "stream", or error which probably means an unconvertible file name (the code above would say "stream", but it is probably not a stream because in practice I don't think apr_file_name_get ever fails so file_name_get can only fail on the string conversion).
> + apr_strerror (status, errbuf, sizeof(errbuf));
> + err = svn_utf_cstring_to_utf8 (&errstr, errbuf, pool);
> + errstr = (err) ? "" : apr_psprintf (pool, ": %s", errstr);
> + svn_error_clear (err);
> +
> + return svn_error_createf (status, NULL, "Can't %s %s%s", op, name,
> errstr);
> +}
> +
> svn_error_t *
> svn_io_file_close (apr_file_t *file, apr_pool_t *pool)
> {
> - apr_status_t status;
> + return do_io_file_wrapper_cleanup
> + ( file, apr_file_close (file),
> + "close", pool);
Inconsistent spacing of parentheses (also repeated below).
> +}
[...]
Otherwise good. I'm glad you are doing this.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 30 20:15:35 2003