[Mono-dev] [PATCH]System.IO.File.Replace

[Paolo Molaro]

On 08/15/07 Bill Holmes wrote:
> Attached is a patch that implements the 2.0 method File.IO.File.Replace.
> 
> http://msdn2.microsoft.com/en-us/library/9etk7xw2.aspx

Please mark your patch attachments as text files instead of as binary
blobs, thanks!

> Index: mono/mono/metadata/file-io.c
> ===================================================================
> --- mono/mono/metadata/file-io.c	(revision 84157)
> +++ mono/mono/metadata/file-io.c	(working copy)
> @@ -10,6 +10,11 @@
>   */
>  
>  #include <config.h>
> +
> +#ifdef PLATFORM_WIN32
> +#define _WIN32_WINNT 0x0500
> +#endif
> +
>  #include <glib.h>
>  #include <string.h>
>  #include <errno.h>
> @@ -384,6 +389,40 @@
>  }
>  
>  MonoBoolean
> +ves_icall_System_IO_MonoIO_ReplaceFile (MonoString *sourceFileName, MonoString *destinationFileName,
> +					MonoString *destinationBackupFileName, MonoBoolean ignoreMetadataErrors,
> +					gint32 *error)
> +{
> +	gboolean ret;
> +	gunichar2 *utf16_sourceFileName=NULL, *utf16_destinationFileName=NULL, *utf16_destinationBackupFileName=NULL;
> +	guint32 replaceFlags = REPLACEFILE_WRITE_THROUGH;
> +	
> +	MONO_ARCH_SAVE_REGS;
> +
> +	if(sourceFileName)
> +		utf16_sourceFileName = mono_string_chars (sourceFileName);
> +
> +	if(destinationFileName)
> +		utf16_destinationFileName = mono_string_chars (destinationFileName);
> +
> +	if(destinationBackupFileName)
> +		utf16_destinationBackupFileName = mono_string_chars (destinationBackupFileName);
> +
> +	*error=ERROR_SUCCESS;

Please make new code follow the mono coding conventions: space before
parens, spaces around operators like = etc.

>  #endif /* _MONO_METADATA_FILEIO_H_ */
> Index: mono/mono/metadata/icall-def.h
> ===================================================================
> --- mono/mono/metadata/icall-def.h	(revision 84157)
> +++ mono/mono/metadata/icall-def.h	(working copy)
> @@ -270,22 +270,25 @@
>  ICALL(MONOIO_16, "Open(string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare,System.IO.FileOptions,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_Open)
>  ICALL(MONOIO_17, "Read(intptr,byte[],int,int,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_Read)
>  ICALL(MONOIO_18, "RemoveDirectory(string,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_RemoveDirectory)
> -ICALL(MONOIO_19, "Seek(intptr,long,System.IO.SeekOrigin,System.IO.MonoIOError&)", ves_icall_System_IO_MonoIO_Seek)

As written at the beginning of this file, do not rename all the entries,
just add the icall in the correctly sorted place. The sequential name is
meanlingless, you could use MONOIO_18M or MONOIO_185 in this case.

> +ICALL(MONOIO_31, "get_DirectorySeparatorChar", ves_icall_System_IO_MonoIO_get_DirectorySeparatorChar)
> +ICALL(MONOIO_32, "get_InvalidPathChars", ves_icall_System_IO_MonoIO_get_InvalidPathChars)
> +ICALL(MONOIO_33, "get_PathSeparator", ves_icall_System_IO_MonoIO_get_PathSeparator)
> +ICALL(MONOIO_34, "get_VolumeSeparatorChar", ves_icall_System_IO_MonoIO_get_VolumeSeparatorChar)
>  
> +
> +

Please don't add random withspace to the code.

> Index: mono/mono/io-layer/io.c
> ===================================================================
> --- mono/mono/io-layer/io.c	(revision 84157)
> +++ mono/mono/io-layer/io.c	(working copy)
> @@ -1835,6 +1835,51 @@
>  	return(ret);
>  }
>  
> +gboolean write_file (int src_fd, int dest_fd, struct stat *st_src, gboolean report_errors)
> +{
> +	int remain, n;
> +	int buf_size = st_src->st_blksize;
> +	char *buf = (char *) alloca (buf_size);
> +
> +	for (;;) {
> +		remain = read (src_fd, buf, buf_size);
> +		
> +		if (remain < 0) {
> +			if (errno == EINTR && !_wapi_thread_cur_apc_pending ()) {
> +				continue;
> +			}
> +			
> +			if (report_errors)
> +				_wapi_set_last_error_from_errno ();
> +			
> +			return(FALSE);
> +		}
> +		
> +		if (remain == 0) {
> +			break;
> +		}
> +
> +		while (remain > 0) {
> +			if ((n = write (dest_fd, buf, remain)) < 0) {
> +				if (errno == EINTR && !_wapi_thread_cur_apc_pending ())
> +					continue;
> +
> +				if (report_errors)
> +					_wapi_set_last_error_from_errno ();
> +#ifdef DEBUG
> +				g_message ("%s: write failed.", __func__);
> +#endif
> +
> +				return (FALSE);
> +			}
> +
> +			remain -= n;
> +		}
> +	}
> +
> +	return (TRUE);
> +}
> +
>  /**
>   * CopyFile:
>   * @name: a pointer to a NULL-terminated unicode string, that names
> @@ -1852,9 +1897,6 @@
>  {
>  	gchar *utf8_src, *utf8_dest;
>  	int src_fd, dest_fd;
> -	int buf_size;
> -	char *buf;
> -	int remain, n;
>  	struct stat st;
>  	
>  	if(name==NULL) {
> @@ -1949,58 +1991,132 @@
>  		return(FALSE);
>  	}
>  	
> -	buf_size = st.st_blksize;
> -	buf = (char *) alloca (buf_size);
> +	if (!write_file (src_fd, dest_fd, &st, TRUE)) {
> +		g_free (utf8_src);
> +		g_free (utf8_dest);
> +		close (src_fd);
> +		close (dest_fd);
> +
> +		return(FALSE);
> +	}
> +
> +	g_free (utf8_src);
> +	g_free (utf8_dest);
> +	close (src_fd);
> +	close (dest_fd);
> +
> +	return(TRUE);

Use a local var to keep the result of write_file ():

	ret = write_file (...);
	// cleanup
	return ret;

This avoids needlessly duplicating all that code.

> +gboolean ReplaceFile (const gunichar2 *replacedFileName, const gunichar2 *replacementFileName,
> +		      const gunichar2 *backupFileName, guint32 replaceFlags, 
> +		      gpointer exclude, gpointer reserved)
> +{
[...]
> +		if (result == -1) {
> +			_wapi_set_last_path_error_from_errno (NULL, utf8_backupFileName);
> +			g_free (utf8_replacedFileName);
> +			g_free (utf8_replacementFileName);
> +			g_free (utf8_backupFileName);
> +			close (backup_fd);
> +			return(FALSE);
> +		}	
[...]
> +			if(replaced_fd == -1) {
> +				g_free (utf8_replacedFileName);
> +				g_free (utf8_replacementFileName);
> +				g_free (utf8_backupFileName);
> +				close (backup_fd);
> +
> +				return(FALSE);
> +			}
[...]
> +		g_free (utf8_replacedFileName);
> +		g_free (utf8_replacementFileName);
> +		g_free (utf8_backupFileName);
> +		close (backup_fd);
> +		close (replaced_fd);
> +		return(FALSE);
[...]
> +	g_free (utf8_replacedFileName);
> +	g_free (utf8_replacementFileName);
> +	g_free (utf8_backupFileName);
> +	close (backup_fd);
> +	
>  	return(TRUE);

The cleanup code is replicated 4 times. In these cases it's better to
use a non-elegant but much more maintainable goto statement.

Thanks!
lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better