Is the attached more like what you're looking for?
Erich Hoover
[EMAIL PROTECTED]
On 2/26/07, Felix Nawothnig <[EMAIL PROTECTED]> wrote:
Erich Hoover wrote:
> I assume you're referring to the file existence check and file delete,
> followed by the actual copy and move. I implemented these checks in
> this manner in order to provide error codes consistent with the
> documentation. I am not incredibly familiar with the Wine internals,
> but it looks to me like the functions called should handle a
> simultaneous call from another thread (without unnecessary code
> duplication). For example, if the "replaced" file goes missing between
> the check and the CopyFileW call then CopyFileW will return an error.
> If the "replaced" file appears between the DeleteFileW call and the
> MoveFileExW call (since the MOVEFILE_REPLACE_EXISTING flag is not set),
> then the MoveFileExW call will fail. The documentation's expected error
> codes for ReplaceFile seem to be consistent with these potential
> outcomes.
Reading MSDN that doesn't seem to be the case. The way I see it all
those errors documented could be caused by file permisions. MSDN doesn't
say anything about race cons.
Some notes about the patch:
+ /* Maker sure the "replacement" file exists before continuing */
+ if(!RtlDoesFileExists_U( lpReplacementFileName ))
+ {
+ SetLastError(ERROR_INVALID_PARAMETER);
+ return FALSE;
+ }
This would probably the right place to lock the file...
+ /*
+ * If the user wants a backup then that needs to be performed first
+ * * Do not fail to backup if "replaced" does not exist
+ */
+ if( lpBackupFileName && RtlDoesFileExists_U( lpReplacedFileName ) )
The call to RtlDoesFileExists_U() is redundant and introduces a race
con. CopyFileW() will check for existence, check GLE afterwards.
+ {
+ /* If an existing backup exists then copy over it */
+ */
+ if(!CopyFileW( lpReplacedFileName, lpBackupFileName, FALSE ))
+ {
+ SetLastError(ERROR_INVALID_PARAMETER);
+ return FALSE;
+ }
+ }
+ /*
+ * Now that the backup has been performed (if requested), copy the
replacement
+ * into place (make sure we can delete the original file first)
+ * * Do not fail if "replaced" does not exist
+ */
+ if(RtlDoesFileExists_U( lpReplacedFileName ) && !DeleteFileW(
lpReplacedFileName ))
Same here, DeleteFile() will check for existence.
+ {
+ SetLastError(ERROR_UNABLE_TO_REMOVE_REPLACED);
+ return FALSE;
+ }
+ if(!MoveFileExW( lpReplacementFileName, lpReplacedFileName, flags ))
Couldn't you just get rid of the DeleteFile, check why MoveFileEx()
failed and get rid of the call to RtlDoesFileExists_U(lpReplacedFileName)?
+ {
+ SetLastError(ERROR_UNABLE_TO_MOVE_REPLACEMENT);
+ return FALSE;
+ }
+ /* All done, file replacement successful */
+ return TRUE;
/**************************************************************************
* ReplaceFileW (KERNEL32.@)
* ReplaceFile (KERNEL32.@)
*/
BOOL WINAPI ReplaceFileW(LPCWSTR lpReplacedFileName,LPCWSTR
lpReplacementFileName,
LPCWSTR lpBackupFileName, DWORD dwReplaceFlags,
LPVOID lpExclude, LPVOID lpReserved)
{
HANDLE hReplaced, hReplacement;
BOOL skipBackup = FALSE;
if(dwReplaceFlags)
FIXME("Ignoring flags %x\n", dwReplaceFlags);
/* First two arguments are mandatory */
if(!lpReplacedFileName || !lpReplacementFileName)
{
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
/*
* Lock the "replacement" file, fail if it does not exist
*/
if((hReplacement = CreateFileW(lpReplacementFileName, GENERIC_READ,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
NULL, OPEN_EXISTING, 0, 0)) == INVALID_HANDLE_VALUE)
{
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
if(!LockFile( hReplacement, 0, 0, 0, 0 ))
{
CloseHandle( hReplacement );
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
/*
* Lock the "replaced" file while we're working.
*/
if((hReplaced = CreateFileW(lpReplacedFileName, GENERIC_READ,
FILE_SHARE_READ | FILE_SHARE_WRITE,
NULL, OPEN_EXISTING, 0, 0)) == INVALID_HANDLE_VALUE)
{
/* If "replaced" does not exist then create it for the lock, but skip
backup */
if((hReplaced = CreateFileW(lpReplacedFileName, GENERIC_READ,
FILE_SHARE_READ | FILE_SHARE_WRITE,
NULL, CREATE_ALWAYS, 0, 0)) == INVALID_HANDLE_VALUE)
{
UnlockFile( hReplacement, 0, 0, 0, 0 );
CloseHandle( hReplacement );
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
skipBackup = TRUE;
}
if(!LockFile( hReplaced, 0, 0, 0, 0 ))
{
UnlockFile( hReplacement, 0, 0, 0, 0 );
CloseHandle( hReplacement );
CloseHandle( hReplaced );
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
/*
* If the user wants a backup then that needs to be performed first
*/
if( lpBackupFileName && !skipBackup )
{
/* If an existing backup exists then copy over it */
if(!CopyFileW( lpReplacedFileName, lpBackupFileName, FALSE ))
{
SetLastError( ERROR_INVALID_PARAMETER );
goto replace_fail;
}
}
/*
* Now that the backup has been performed (if requested), copy the
replacement
* into place
*/
if(!CopyFileW( lpReplacementFileName, lpReplacedFileName, FALSE ))
{
/* Assume that all access denied errors are a write problem. */
if(GetLastError() == ERROR_ACCESS_DENIED)
SetLastError( ERROR_UNABLE_TO_REMOVE_REPLACED );
else
SetLastError( ERROR_UNABLE_TO_MOVE_REPLACEMENT );
goto replace_fail;
}
/* Delete the replacement file */
if(!DeleteFileW( lpReplacementFileName ))
{
SetLastError( ERROR_INVALID_PARAMETER );
return FALSE;
}
/* Unlock the handles */
UnlockFile( hReplacement, 0, 0, 0, 0 );
CloseHandle( hReplacement );
UnlockFile( hReplaced, 0, 0, 0, 0 );
CloseHandle( hReplaced );
/* All done, file replacement successful */
return TRUE;
replace_fail:
UnlockFile( hReplacement, 0, 0, 0, 0 );
CloseHandle( hReplacement );
UnlockFile( hReplaced, 0, 0, 0, 0 );
CloseHandle( hReplaced );
return FALSE;
}
