Re: kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

(CC-ing wine-devel again)

Erich Hoover wrote:
> > The "right" way would probably to do the copying yourself by
> > read/write.. but I dunno.
>
> Except that it would ignore the permissions issues that have already 
> been coded into the copy routines (and any updates that may eventually

No, CreateFile (and friends) does the permissions checks (which you 
would still have to call).
> be made to these routines).  Also, it seems to me that MSDN is 
> describing the list of function calls that ReplaceFile actually makes.

No it doesn't?

Your other objections were right. Your last version follows for wine-devel.
> /**************************************************************************
> -
>       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)
> {
>     return FALSE;
> }
> if(!LockFile( hReplacement, 0, 0, 0, 0 ))
> {
>     CloseHandle( hReplacement );
>     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 );
>         return FALSE;
>     }
>     skipBackup = TRUE;
> }
> if(!LockFile( hReplaced, 0, 0, 0, 0 ))
> {
>     UnlockFile( hReplacement, 0, 0, 0, 0 );
>     CloseHandle( hReplacement );
>     CloseHandle( hReplaced );
>     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 ))
>         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 ))
>     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;
> }

kernel32: Implement ReplaceFileA/ReplaceFileW

[Erich Hoover]

Oops, I must have hit the wrong reply button.
> > > The "right" way would probably to do the copying yourself by
> > > read/write.. but I dunno.
> >
> > Except that it would ignore the permissions issues that have already
> > been coded into the copy routines (and any updates that may eventually
>
> No, CreateFile (and friends) does the permissions checks (which you
> would still have to call).

That was worded poorly, Copy/Move already handle copying file attributes and
I imagine would eventually implement copying the access control list
information.  Implementing ReplaceFile as calls to either Copy or Move takes
these issues into account.
> > be made to these routines).  Also, it seems to me that MSDN is
> > describing the list of function calls that ReplaceFile actually makes.
>
> No it doesn't?
> From my perspective the "remarks" section discusses the implementation of

ReplaceFile as calls to existing API functions (as a sort of macro) as a
convenience.  Granted, it discusses moving files instead of copying but that
conflicted with the file locking.

Should I make a new patch file with the previously attached changes or would
you prefer that the function be implemented in the read/write manner?
Re-implementing the function in such a fashion would make it possible to do
a more move-like operation while maintaining the file locking, but would
lose out on any future improvements to CopyFile.

Erich Hoover
[e-mail]

On 2/28/07, Felix Nawothnig  wrote:
> (CC-ing wine-devel again)
>
> Erich Hoover wrote:
> > > The "right" way would probably to do the copying yourself by
> > > read/write.. but I dunno.
> >
> > Except that it would ignore the permissions issues that have already
> > been coded into the copy routines (and any updates that may eventually
>
> No, CreateFile (and friends) does the permissions checks (which you
> would still have to call).
> > be made to these routines).  Also, it seems to me that MSDN is
> > describing the list of function calls that ReplaceFile actually makes.
>
> No it doesn't?
>
> Your other objections were right. Your last version follows for
> wine-devel.
> >
>
> /**************************************************************************
> > -
> >       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)
> > {
> >     return FALSE;
> > }
> > if(!LockFile( hReplacement, 0, 0, 0, 0 ))
> > {
> >     CloseHandle( hReplacement );
> >     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 );
> >         return FALSE;
> >     }
> >     skipBackup = TRUE;
> > }
> > if(!LockFile( hReplaced, 0, 0, 0, 0 ))
> > {
> >     UnlockFile( hReplacement, 0, 0, 0, 0 );
> >     CloseHandle( hReplacement );
> >     CloseHandle( hReplaced );
> >     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 ))
> >         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 ))
> >     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;
> > }

kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

Erich Hoover wrote:
> > > > The "right" way would probably to do the copying yourself by
> > > > read/write.. but I dunno.
> > >
> > > Except that it would ignore the permissions issues that have already
> > > been coded into the copy routines (and any updates that may eventually
> >
> > No, CreateFile (and friends) does the permissions checks (which you
> > would still have to call).
>
> That was worded poorly, Copy/Move already handle copying file attributes 
> and I imagine would eventually implement copying the access control list 
> information.  Implementing ReplaceFile as calls to either Copy or Move 
> takes these issues into account.

I see your point. However, since the function you are implementing is in 
kernel32 anyway you could abstract it away and make both functions 
(CopyFile and ReplaceFile) call some internal function. That way you 
would get rid of the locking completly which is argueably somewhat ugly.

Felix

kernel32: Implement ReplaceFileA/ReplaceFileW

[Erich Hoover]

> I see your point. However, since the function you are implementing is in
> kernel32 anyway you could abstract it away and make both functions
> (CopyFile and ReplaceFile) call some internal function. That way you
> would get rid of the locking completly which is argueably somewhat ugly.

On closer inspection, CreateFile actually seems to take care of this with
the "security attributes" and "template" parameters.  It does not look like
the security attributes are implemented yet (or the call to get the security
attributes for a file) but since CreateFile already handles this it seems
unnecessary to add a new break-out function.  You will likely find the
attached more to your liking.

Erich Hoover
[e-mail]

On 2/28/07, Felix Nawothnig  wrote:
> Erich Hoover wrote:
> > > > > The "right" way would probably to do the copying yourself by
> > > > > read/write.. but I dunno.
> > > >
> > > > Except that it would ignore the permissions issues that have already
> > > > been coded into the copy routines (and any updates that may eventually
> > >
> > > No, CreateFile (and friends) does the permissions checks (which you
> > > would still have to call).
> >
> > That was worded poorly, Copy/Move already handle copying file attributes
> > and I imagine would eventually implement copying the access control list
> > information.  Implementing ReplaceFile as calls to either Copy or Move
> > takes these issues into account.
>
> I see your point. However, since the function you are implementing is in
> kernel32 anyway you could abstract it away and make both functions
> (CopyFile and ReplaceFile) call some internal function. That way you
> would get rid of the locking completly which is argueably somewhat ugly.
>
> Felix

kernel32: Implement ReplaceFileA/ReplaceFileW

[Felix Nawothnig]

Erich Hoover wrote:
> > I see your point. However, since the function you are implementing is in
> > kernel32 anyway you could abstract it away and make both functions
> > (CopyFile and ReplaceFile) call some internal function. That way you
> > would get rid of the locking completly which is argueably somewhat ugly.
>
> On closer inspection, CreateFile actually seems to take care of this 
> with the "security attributes" and "template" parameters.  It does not 
> look like the security attributes are implemented yet (or the call to 
> get the security attributes for a file) but since CreateFile already 
> handles this it seems unnecessary to add a new break-out function.  You 
> will likely find the attached more to your liking.

Doesn't matter what I like, in the end it's up to Alexandre.

But yes, looks much better to me (although I'd still say it makes sense 
to abstract away the actually copying in a subroutine).

Felix