From 85f4971f850c0110915ba862922c1536ebe506c2 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Fri, 30 Dec 2011 20:09:06 -0500 Subject: [PATCH] Windows: renames that overwrite existing target The Windows client up to this point has never correctly implemented directory renames. For the longest time it assumed that the file server would not replace a pre-existing target. As a result, when the target name was already in use the contents of the directory would end up with the target name existing but its previous file id associated with it. A second problem was that lookups for the source and target names were not performed while the directory (or directories) were exclusively held to ensure that competing changes could not occur. This patchset corrects both issues in cm_Rename() and adjusts the redirector interface to match the new behavior. Change-Id: I4f5cff7debcf9925947ac3fc6931565acb57ebd9 Reviewed-on: http://gerrit.openafs.org/6457 Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- src/WINNT/afsd/cm_vnodeops.c | 192 +++++++++++++++++++++--------------- src/WINNT/afsrdr/user/RDRFunction.c | 22 +++-- 2 files changed, 126 insertions(+), 88 deletions(-) diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c index 1e52b98..fadcc81 100644 --- a/src/WINNT/afsd/cm_vnodeops.c +++ b/src/WINNT/afsd/cm_vnodeops.c @@ -3613,14 +3613,14 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep cm_req_t *reqp) { cm_conn_t *connp; - long code; + long code = 0; AFSFid oldDirAFSFid; AFSFid newDirAFSFid; - int didEnd; AFSFetchStatus updatedOldDirStatus; AFSFetchStatus updatedNewDirStatus; AFSVolSync volSync; - int oneDir; + int oneDir = 0; + int bTargetExists = 0; struct rx_connection * rxconnp; cm_dirOp_t oldDirOp; cm_fid_t fileFid; @@ -3628,7 +3628,8 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep cm_dirOp_t newDirOp; fschar_t * newNamep = NULL; int free_oldNamep = FALSE; - cm_scache_t *oldScp = NULL, *newScp = NULL; + cm_scache_t *oldScp = NULL, *oldTargetScp = NULL; + int rpc_skipped = 0; memset(&volSync, 0, sizeof(volSync)); @@ -3637,54 +3638,19 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep cm_ClientStrLen(cNewNamep) == 0) return CM_ERROR_INVAL; - /* - * Before we permit the operation, make sure that we do not already have - * an object in the destination directory that has a case-insensitive match - * for this name UNLESS the matching object is the object we are renaming. - */ - code = cm_Lookup(oldDscp, cOldNamep, 0, userp, reqp, &oldScp); - if (code) { - osi_Log2(afsd_logp, "cm_Rename oldDscp 0x%p cOldName %S old name lookup failed", - oldDscp, osi_LogSaveStringW(afsd_logp, cOldNamep)); - goto done; - } - - /* Case sensitive lookup. If this succeeds we are done. */ - code = cm_Lookup(newDscp, cNewNamep, 0, userp, reqp, &newScp); - if (code) { - /* - * Case insensitive lookup. If this succeeds, it could have found the - * same file with a name that differs only by case or it could be a - * different file entirely. - */ - code = cm_Lookup(newDscp, cNewNamep, CM_FLAG_CASEFOLD, userp, reqp, &newScp); - if (code == 0) { - /* found a matching object with the new name */ - if (cm_FidCmp(&oldScp->fid, &newScp->fid)) { - /* and they don't match so return an error */ - osi_Log2(afsd_logp, "cm_Rename newDscp 0x%p cNewName %S new name already exists", - newDscp, osi_LogSaveStringW(afsd_logp, cNewNamep)); - code = CM_ERROR_EXISTS; - } - cm_ReleaseSCache(newScp); - newScp = NULL; - } else if (code == CM_ERROR_AMBIGUOUS_FILENAME) { - code = CM_ERROR_EXISTS; - } else { - /* The target does not exist. Clear the error and perform the rename. */ - code = 0; - } + /* check for identical names */ + if (oldDscp == newDscp && + cm_ClientStrCmp(cOldNamep, cNewNamep) == 0) { + osi_Log2(afsd_logp, "cm_Rename oldDscp 0x%p newDscp 0x%p CM_ERROR_RENAME_IDENTICAL", + oldDscp, newDscp); + return CM_ERROR_RENAME_IDENTICAL; } /* Check for RO volume */ - if (code == 0 && - (oldDscp->flags & CM_SCACHEFLAG_RO) || (newDscp->flags & CM_SCACHEFLAG_RO)) { - code = CM_ERROR_READONLY; + if ((oldDscp->flags & CM_SCACHEFLAG_RO) || (newDscp->flags & CM_SCACHEFLAG_RO)) { + return CM_ERROR_READONLY; } - if (code) - goto done; - if (oldNamep == NULL) { code = -1; #ifdef USE_BPLUS @@ -3704,21 +3670,12 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep } } - /* before starting the RPC, mark that we're changing the directory data, * so that someone who does a chmod on the dir will wait until our call * completes. We do this in vnode order so that we don't deadlock, * which makes the code a little verbose. */ if (oldDscp == newDscp) { - /* check for identical names */ - if (cm_ClientStrCmp(cOldNamep, cNewNamep) == 0) { - osi_Log2(afsd_logp, "cm_Rename oldDscp 0x%p newDscp 0x%p CM_ERROR_RENAME_IDENTICAL", - oldDscp, newDscp); - code = CM_ERROR_RENAME_IDENTICAL; - goto done; - } - oneDir = 1; cm_BeginDirOp(oldDscp, userp, reqp, CM_DIRLOCK_NONE, CM_DIROP_FLAG_NONE, &oldDirOp); @@ -3820,7 +3777,55 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep if (code) goto done; - didEnd = 0; + /* + * The source and destination directories are now locked and no other local + * changes can occur. + * + * Before we permit the operation, make sure that we do not already have + * an object in the destination directory that has a case-insensitive match + * for this name UNLESS the matching object is the object we are renaming. + */ + code = cm_Lookup(oldDscp, cOldNamep, 0, userp, reqp, &oldScp); + if (code) { + osi_Log2(afsd_logp, "cm_Rename oldDscp 0x%p cOldName %S old name lookup failed", + oldDscp, osi_LogSaveStringW(afsd_logp, cOldNamep)); + rpc_skipped = 1; + goto post_rpc; + } + + /* Case sensitive lookup. If this succeeds we are done. */ + code = cm_Lookup(newDscp, cNewNamep, 0, userp, reqp, &oldTargetScp); + if (code) { + /* + * Case insensitive lookup. If this succeeds, it could have found the + * same file with a name that differs only by case or it could be a + * different file entirely. + */ + code = cm_Lookup(newDscp, cNewNamep, CM_FLAG_CASEFOLD, userp, reqp, &oldTargetScp); + if (code == 0) { + /* found a matching object with the new name */ + if (cm_FidCmp(&oldScp->fid, &oldTargetScp->fid)) { + /* and they don't match so return an error */ + osi_Log2(afsd_logp, "cm_Rename newDscp 0x%p cNewName %S new name already exists", + newDscp, osi_LogSaveStringW(afsd_logp, cNewNamep)); + code = CM_ERROR_EXISTS; + } + cm_ReleaseSCache(oldTargetScp); + oldTargetScp = NULL; + } else if (code == CM_ERROR_AMBIGUOUS_FILENAME) { + code = CM_ERROR_EXISTS; + } else { + /* The target does not exist. Clear the error and perform the rename. */ + code = 0; + } + } else { + bTargetExists = 1; + } + + if (code) { + rpc_skipped = 1; + goto post_rpc; + } newNamep = cm_ClientStringToFsStringAlloc(cNewNamep, -1, NULL); @@ -3858,6 +3863,7 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep else osi_Log0(afsd_logp, "CALL Rename SUCCESS"); + post_rpc: /* update the individual stat cache entries for the directories */ if (oldDirOp.scp) { lock_ObtainWrite(&oldDirOp.scp->dirlock); @@ -3870,6 +3876,13 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep userp, reqp, CM_MERGEFLAG_DIROP); if (cm_CheckDirOpForSingleChange(&oldDirOp)) { lock_ReleaseWrite(&oldDscp->rw); + if (bTargetExists && oneDir) { + diropCode = cm_DirDeleteEntry(&oldDirOp, newNamep); +#ifdef USE_BPLUS + cm_BPlusDirDeleteEntry(&oldDirOp, cNewNamep); +#endif + } + #ifdef USE_BPLUS diropCode = cm_BPlusDirLookup(&oldDirOp, cOldNamep, &fileFid); if (diropCode == CM_ERROR_INEXACT_MATCH) @@ -3896,7 +3909,8 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep lock_ObtainWrite(&oldDscp->rw); } } else { - InterlockedDecrement(&oldDscp->activeRPCs); + if (!rpc_skipped) + InterlockedDecrement(&oldDscp->activeRPCs); } cm_SyncOpDone(oldDscp, NULL, CM_SCACHESYNC_STOREDATA); lock_ReleaseWrite(&oldDscp->rw); @@ -3913,32 +3927,31 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep if (code == 0) { cm_MergeStatus(NULL, newDscp, &updatedNewDirStatus, &volSync, userp, reqp, CM_MERGEFLAG_DIROP); -#if 0 + /* - * The following optimization does not work. - * When the file server processed a RXAFS_Rename() request the - * FID of the object being moved between directories is not - * preserved. The client does not know the new FID nor the - * version number of the target. Not only can we not create - * the directory entry in the new directory, but we can't - * preserve the cached data for the file. It must be re-read - * from the file server. - jaltman, 2009/02/20 - */ - /* we only make the local change if we successfully made + * we only make the local change if we successfully made * the change in the old directory AND there was only one * change in the new directory */ if (diropCode == 0 && cm_CheckDirOpForSingleChange(&newDirOp)) { lock_ReleaseWrite(&newDscp->rw); + + if (bTargetExists && !oneDir) { + diropCode = cm_DirDeleteEntry(&newDirOp, newNamep); +#ifdef USE_BPLUS + cm_BPlusDirDeleteEntry(&newDirOp, cNewNamep); +#endif + } + cm_DirCreateEntry(&newDirOp, newNamep, &fileFid); #ifdef USE_BPLUS cm_BPlusDirCreateEntry(&newDirOp, cNewNamep, &fileFid); #endif lock_ObtainWrite(&newDscp->rw); } -#endif /* 0 */ } else { - InterlockedIncrement(&newDscp->activeRPCs); + if (!rpc_skipped) + InterlockedIncrement(&newDscp->activeRPCs); } cm_SyncOpDone(newDscp, NULL, CM_SCACHESYNC_STOREDATA); lock_ReleaseWrite(&newDscp->rw); @@ -3946,22 +3959,37 @@ long cm_Rename(cm_scache_t *oldDscp, fschar_t *oldNamep, clientchar_t *cOldNamep cm_EndDirOp(&newDirOp); } - /* - * After the rename the file server has invalidated the callbacks - * on the file that was moved nor do we have a directory reference - * to it anymore. - */ - lock_ObtainWrite(&oldScp->rw); - cm_DiscardSCache(oldScp); - lock_ReleaseWrite(&oldScp->rw); + if (code == 0) { + /* + * After the rename the file server has invalidated the callbacks + * on the file that was moved and destroyed any target file. + */ + lock_ObtainWrite(&oldScp->rw); + cm_DiscardSCache(oldScp); + lock_ReleaseWrite(&oldScp->rw); + + if (RDR_Initialized) + RDR_InvalidateObject(oldScp->fid.cell, oldScp->fid.volume, oldScp->fid.vnode, oldScp->fid.unique, + oldScp->fid.hash, oldScp->fileType, AFS_INVALIDATE_CALLBACK); + + if (oldTargetScp) { + lock_ObtainWrite(&oldTargetScp->rw); + cm_DiscardSCache(oldTargetScp); + lock_ReleaseWrite(&oldTargetScp->rw); + + if (RDR_Initialized) + RDR_InvalidateObject(oldTargetScp->fid.cell, oldTargetScp->fid.volume, oldTargetScp->fid.vnode, oldTargetScp->fid.unique, + oldTargetScp->fid.hash, oldTargetScp->fileType, AFS_INVALIDATE_CALLBACK); + } + } - if (RDR_Initialized) - RDR_InvalidateObject(oldScp->fid.cell, oldScp->fid.volume, oldScp->fid.vnode, oldScp->fid.unique, - oldScp->fid.hash, oldScp->fileType, AFS_INVALIDATE_CALLBACK); done: if (oldScp) cm_ReleaseSCache(oldScp); + if (oldTargetScp) + cm_ReleaseSCache(oldTargetScp); + if (free_oldNamep) free(oldNamep); diff --git a/src/WINNT/afsrdr/user/RDRFunction.c b/src/WINNT/afsrdr/user/RDRFunction.c index e0efe83..bc9ab61 100644 --- a/src/WINNT/afsrdr/user/RDRFunction.c +++ b/src/WINNT/afsrdr/user/RDRFunction.c @@ -2216,8 +2216,12 @@ RDR_RenameFileEntry( IN cm_user_t *userp, DWORD TargetFileNameLength = pRenameCB->TargetNameLength; cm_fid_t SourceParentFid; cm_fid_t TargetParentFid; + cm_fid_t SourceFid; + cm_fid_t OrigTargetFid = {0,0,0,0,0}; + cm_fid_t TargetFid; cm_scache_t * oldDscp; cm_scache_t * newDscp; + cm_dirOp_t dirop; wchar_t shortName[13]; wchar_t SourceFileName[260]; wchar_t TargetFileName[260]; @@ -2341,11 +2345,17 @@ RDR_RenameFileEntry( IN cm_user_t *userp, return; } + /* Obtain the original FID just for debugging purposes */ + code = cm_BeginDirOp( oldDscp, userp, &req, CM_DIRLOCK_READ, CM_DIROP_FLAG_NONE, &dirop); + if (code == 0) { + code = cm_BPlusDirLookup(&dirop, SourceFileName, &SourceFid); + code = cm_BPlusDirLookup(&dirop, TargetFileName, &OrigTargetFid); + cm_EndDirOp(&dirop); + } + code = cm_Rename( oldDscp, NULL, SourceFileName, newDscp, TargetFileName, userp, &req); if (code == 0) { - cm_dirOp_t dirop; - cm_fid_t targetFid; cm_scache_t *scp = 0; DWORD dwRemaining; @@ -2361,7 +2371,7 @@ RDR_RenameFileEntry( IN cm_user_t *userp, code = cm_BeginDirOp( newDscp, userp, &req, CM_DIRLOCK_READ, CM_DIROP_FLAG_NONE, &dirop); if (code == 0) { - code = cm_BPlusDirLookup(&dirop, TargetFileName, &targetFid); + code = cm_BPlusDirLookup(&dirop, TargetFileName, &TargetFid); cm_EndDirOp(&dirop); } @@ -2375,10 +2385,10 @@ RDR_RenameFileEntry( IN cm_user_t *userp, } osi_Log4(afsd_logp, "RDR_RenameFileEntry Target FID cell=0x%x vol=0x%x vn=0x%x uniq=0x%x", - targetFid.cell, targetFid.volume, - targetFid.vnode, targetFid.unique); + TargetFid.cell, TargetFid.volume, + TargetFid.vnode, TargetFid.unique); - code = cm_GetSCache(&targetFid, &scp, userp, &req); + code = cm_GetSCache(&TargetFid, &scp, userp, &req); if (code) { osi_Log1(afsd_logp, "RDR_RenameFileEntry cm_GetSCache target failed code 0x%x", code); smb_MapNTError(cm_MapRPCError(code, &req), &status, TRUE); -- 1.9.4