Thanks for the patch submission. However, I have several concerns about it.
B. Wells wrote:
> For this workaround, I followed the same pattern as the other Mac OS X
> hack in the source file.
Where is that? I can't find one.
Which version of this source file are you looking at? The trunk version has
svn_io_file_create() at line 960, not around 770.
> A simpler patch would have been to prevent zero
> byte write operations for all newly created files. But it may be that
> such a write operation is needed on some file systems to properly create
> the new file. Thus this patch compiles specificly for Mac OS X and only
> alters functionality when creating files on SMB shares.
I don't think there is any need to perform the zero-byte write on any systems,
but I am not sure.
> Of course, this patch is really useful when APR is also patched to
> support file locking on SMB shares. I wrote such a patch several months
> ago and posted it on my website.
>
> http://homepage.mac.com/brianwells/.Public/svn_darwin_flock_patch
>
> I should clean it up and post it to the dev@apr.apache.org mailing list
> to see if they are interested in it. Or perhaps even better would be to
> redo it as a patch for svn_io_file_lock2 so that the patches stay with
> Subversion.
If it's doing something that a portability layer should be doing, please post
it to the APR development list.
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (.../vendor/subversion/current) (revision 31)
> +++ subversion/libsvn_subr/io.c (.../trunk/subversion) (revision 31)
> @@ -770,6 +770,29 @@
> return SVN_NO_ERROR;
> }
(One of the three lines of context seems to be missing from the patch here -
maybe a problem with your mail program. If this always happens, consider
attaching the patch (with a text MIME type) instead of pasting it.)
> +
> +/*
> + Some of the most recent releases of Mac OS X 10.4 (10.4.7 for sure) have a
> + bug that results in an I/O error when an attempt is made to perform a write
> + operation of zero bytes to a file on a mounted SMB share. This is a big
> + problem when Subversion calls svn_io_file_create() to create empty files.
> +
> + This workaround will avoid write operations of zero bytes if the file being
> + created is on an SMB share.
> + */
> +#if defined(__APPLE__) && defined(__MACH__)
> +#include <sys/param.h>
> +#include <sys/mount.h>
> +#define MACOSX_SMBWRITE_HACK (file,contents) \
> +struct statfs sfb; \
> +if (strlen(contents) != 0 \
> + || statfs(file, &sfb) == -1 \
> + || apr_strnatcmp(sfb.f_fstypename,"smbfs") != 0)
Have you tested this? The space before the macro's parentheses should mean it
defines as a macro without an argument list, and thus I'd expect compilation to
fail.
The "struct" definition will appear after statements, which is not valid ANSI
C'89 even if your compiler accepts it.
Repeating the "strlen" operation here is inefficient.
> +#else
> +#define MACOSX_SMBWRITE_HACK(file, contents) do {} while (0);
This definition might as well be blank: there's no need for it to evaluate to a
statement when the other version doesn't.
> +#endif
> +
> +
> svn_error_t *svn_io_file_create (const char *file,
> const char *contents,
> apr_pool_t *pool)
> @@ -781,6 +804,7 @@
> (APR_WRITE | APR_CREATE | APR_EXCL),
> APR_OS_DEFAULT,
> pool));
> + MACOSX_SMBWRITE_HACK(file,contents)
> SVN_ERR (svn_io_file_write_full (f, contents, strlen (contents),
> &written, pool));
> SVN_ERR (svn_io_file_close (f, pool));
Is this the only place where we might perform a zero-byte write? Wouldn't it
be better to put this hack in svn_io_file_write_full() and svn_io_file_write()?
Or, better still, in APR?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 29 21:31:11 2006