db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-03-05 15:05:48 +02:00
Code Issues Releases Activity

Fix SFTP renaming failure when file already exists.

Browse Source 
Update error handling for libssh2_sftp_rename_ex() in storageWriteSftpClose() when a file already exists. 

The SFTP servers used during development and testing never returned LIBSSH2_FX_FILE_ALREADY_EXISTS, rather they returned LIBSSH2_FX_FAILURE when a file already existed. However, it is clear that some SFTP servers use LIBSSH2_FX_FILE_ALREADY_EXISTS so add support.
This commit is contained in:
Reid Thompson 2024-07-04 05:53:07 -04:00 committed by GitHub
parent edd61636a9
commit d6f0bf88af
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 75 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
doc/xml/release/2024/2.53.xml
View File

@ -20,6 +20,19 @@
<p>Fix issue with files larger on the replica than on the primary.</p>
</release-item>
<release-item>
<github-issue id="2376"/>
<github-pull-request id="2377"/>
<release-item-contributor-list>
<release-item-ideator id="ahmed112212"/>
<release-item-contributor id="reid.thompson"/>
<release-item-reviewer id="david.steele"/>
</release-item-contributor-list>
<p>Fix <proper>SFTP</proper> renaming failure when file already exists.</p>
</release-item>
</release-bug-list>
<release-improvement-list>

5
doc/xml/release/contributor.xml
View File

@ -35,6 +35,11 @@
<contributor-id type="github">disco-stu</contributor-id>
</contributor>
<contributor id="ahmed112212">
<contributor-name-display>ahmed112212</contributor-name-display>
<contributor-id type="github">ahmed112212</contributor-id>
</contributor>
<contributor id="aleksandr.rogozin">
<contributor-name-display>Aleksandr Rogozin</contributor-name-display>
<contributor-id type="github">arogozin</contributor-id>

6
src/storage/sftp/storage.c
View File

@ -901,7 +901,7 @@ storageSftpPathCreate(
const uint64_t sftpErrno = libssh2_sftp_last_error(this->sftpSession);
// libssh2 may return LIBSSH2_FX_FAILURE if the directory already exists
if (sftpErrno == LIBSSH2_FX_FAILURE)
if (sftpErrno == LIBSSH2_FX_FAILURE || sftpErrno == LIBSSH2_FX_FILE_ALREADY_EXISTS)
{
// Check if the directory already exists
LIBSSH2_SFTP_ATTRIBUTES attr;
@ -921,7 +921,7 @@ storageSftpPathCreate(
{
storageSftpEvalLibSsh2Error(
rc, libssh2_sftp_last_error(this->sftpSession), &PathCreateError,
strNewFmt("unable to create path '%s': path already exists", strZ(path)), NULL);
strNewFmt("sftp error unable to create path '%s': path already exists", strZ(path)), NULL);
}
}
// If the parent path does not exist then create it if allowed
@ -934,7 +934,7 @@ storageSftpPathCreate(
strFree(pathParent);
}
else if (sftpErrno != LIBSSH2_FX_FILE_ALREADY_EXISTS || errorOnExists)
else
{
storageSftpEvalLibSsh2Error(
rc, sftpErrno, &PathCreateError, strNewFmt("sftp error unable to create path '%s'", strZ(path)), NULL);

7
src/storage/sftp/write.c
View File

@ -313,8 +313,11 @@ storageWriteSftpClose(THIS_VOID)
if (rc == LIBSSH2_ERROR_EAGAIN)
THROW_FMT(FileCloseError, "timeout renaming file '%s'", strZ(this->nameTmp));
const uint64_t sftpErr = libssh2_sftp_last_error(this->sftpSession);
// Some/most sftp servers will not rename over an existing file, in testing this returned LIBSSH2_FX_FAILURE
if (rc == LIBSSH2_ERROR_SFTP_PROTOCOL && libssh2_sftp_last_error(this->sftpSession) == LIBSSH2_FX_FAILURE)
if (rc == LIBSSH2_ERROR_SFTP_PROTOCOL &&
(sftpErr == LIBSSH2_FX_FAILURE || sftpErr == LIBSSH2_FX_FILE_ALREADY_EXISTS))
{
// Remove the existing file and retry the rename
storageWriteSftpUnlinkExisting(this);
@ -323,7 +326,7 @@ storageWriteSftpClose(THIS_VOID)
else
{
storageSftpEvalLibSsh2Error(
rc, libssh2_sftp_last_error(this->sftpSession), &FileCloseError,
rc, sftpErr, &FileCloseError,
strNewFmt("unable to move '%s' to '%s'", strZ(this->nameTmp), strZ(this->interface.name)), NULL);
}
}

52
test/src/module/storage/sftpTest.c
View File

@ -4113,7 +4113,7 @@ testRun(void)
TEST_ERROR(storagePathCreateP(storageTest, STRDEF("sub1")), PathCreateError, "timeout stat'ing path '" TEST_PATH "/sub1'");
TEST_ERROR(
storagePathCreateP(storageTest, STRDEF("sub1"), .errorOnExists = true), PathCreateError,
"unable to create path '" TEST_PATH "/sub1': path already exists");
"sftp error unable to create path '" TEST_PATH "/sub1': path already exists");
// NOTE: if operating against an actual sftp server, a neutral umask is required to get the proper permissions.
// Without the neutral umask, permissions were 0775.
@ -4155,10 +4155,13 @@ testRun(void)
{.function = HRNLIBSSH2_SFTP_MKDIR_EX, .param = "[\"" TEST_PATH "/subfail\",488]",
.resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_FILE_ALREADY_EXISTS},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/subfail\",0]", .resultInt = LIBSSH2_ERROR_NONE},
// Error on already exists
{.function = HRNLIBSSH2_SFTP_MKDIR_EX, .param = "[\"" TEST_PATH "/subfail\",488]",
.resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_FILE_ALREADY_EXISTS},
{.function = HRNLIBSSH2_SFTP_STAT_EX, .param = "[\"" TEST_PATH "/subfail\",0]", .resultInt = LIBSSH2_ERROR_NONE},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_FILE_ALREADY_EXISTS},
HRNLIBSSH2_MACRO_SHUTDOWN()
});
@ -4182,7 +4185,7 @@ testRun(void)
TEST_RESULT_VOID(storagePathCreateP(storageTest, STRDEF("subfail")), "do not throw error on already exists");
TEST_ERROR(
storagePathCreateP(storageTest, STRDEF("subfail"), .errorOnExists = true), PathCreateError,
"sftp error unable to create path '" TEST_PATH "/subfail': libssh2 error [-31]: sftp error [11]");
"sftp error unable to create path '" TEST_PATH "/subfail': path already exists");
memContextFree(objMemContext((StorageSftp *)storageDriver(storageTest)));
#else
@ -5245,7 +5248,6 @@ testRun(void)
.param = "[\"" TEST_PATH "/sub1/testfile.pgbackrest.tmp\",\"" TEST_PATH "/sub1/testfile\",7]",
.resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_CONNECTION_LOST},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_CONNECTION_LOST},
HRNLIBSSH2_MACRO_SHUTDOWN()
});
@ -5269,6 +5271,50 @@ testRun(void)
memContextFree(objMemContext((StorageSftp *)storageDriver(storageTest)));
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("sftp remove existing file on rename - LIBSSH2_FX_FILE_ALREADY_EXISTS");
hrnLibSsh2ScriptSet((HrnLibSsh2 [])
{
HRNLIBSSH2_MACRO_STARTUP(),
{.function = HRNLIBSSH2_SFTP_OPEN_EX, .resultNull = true,
.param = "[\"" TEST_PATH "/sub1/testfile.pgbackrest.tmp\",26,416,0]"},
{.function = HRNLIBSSH2_SESSION_LAST_ERRNO, .resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SESSION_LAST_ERRNO, .resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_NO_SUCH_FILE},
{.function = HRNLIBSSH2_SFTP_MKDIR_EX, .param = "[\"" TEST_PATH "/sub1\",488]"},
{.function = HRNLIBSSH2_SFTP_OPEN_EX, .param = "[\"" TEST_PATH "/sub1/testfile.pgbackrest.tmp\",26,416,0]"},
{.function = HRNLIBSSH2_SFTP_FSYNC, .resultInt = LIBSSH2_ERROR_NONE},
{.function = HRNLIBSSH2_SFTP_CLOSE_HANDLE, .resultInt = LIBSSH2_ERROR_NONE},
{.function = HRNLIBSSH2_SFTP_RENAME_EX,
.param = "[\"" TEST_PATH "/sub1/testfile.pgbackrest.tmp\",\"" TEST_PATH "/sub1/testfile\",7]",
.resultInt = LIBSSH2_ERROR_SFTP_PROTOCOL},
{.function = HRNLIBSSH2_SFTP_LAST_ERROR, .resultUInt = LIBSSH2_FX_FILE_ALREADY_EXISTS},
{.function = HRNLIBSSH2_SFTP_UNLINK_EX, .param = "[\"" TEST_PATH "/sub1/testfile\"]", .resultInt = LIBSSH2_ERROR_NONE},
{.function = HRNLIBSSH2_SFTP_RENAME_EX,
.param = "[\"" TEST_PATH "/sub1/testfile.pgbackrest.tmp\",\"" TEST_PATH "/sub1/testfile\",7]",
.resultInt = LIBSSH2_ERROR_NONE},
HRNLIBSSH2_MACRO_SHUTDOWN()
});
storageTest = storageSftpNewP(
cfgOptionIdxStr(cfgOptRepoPath, repoIdx), cfgOptionIdxStr(cfgOptRepoSftpHost, repoIdx),
cfgOptionIdxUInt(cfgOptRepoSftpHostPort, repoIdx), cfgOptionIdxStr(cfgOptRepoSftpHostUser, repoIdx),
cfgOptionUInt64(cfgOptIoTimeout), cfgOptionIdxStr(cfgOptRepoSftpPrivateKeyFile, repoIdx),
cfgOptionIdxStrId(cfgOptRepoSftpHostKeyHashType, repoIdx), .modeFile = STORAGE_MODE_FILE_DEFAULT,
.modePath = STORAGE_MODE_PATH_DEFAULT, .keyPub = cfgOptionIdxStrNull(cfgOptRepoSftpPublicKeyFile, repoIdx),
.keyPassphrase = cfgOptionIdxStrNull(cfgOptRepoSftpPrivateKeyPassphrase, repoIdx),
.hostFingerprint = cfgOptionIdxStrNull(cfgOptRepoSftpHostFingerprint, repoIdx),
.hostKeyCheckType = cfgOptionIdxStrId(cfgOptRepoSftpHostKeyCheckType, repoIdx),
.knownHosts = strLstNewVarLst(cfgOptionIdxLst(cfgOptRepoSftpKnownHost, repoIdx)), .write = true);
TEST_ASSIGN(file, storageNewWriteP(storageTest, fileName), "new write file (defaults)");
TEST_RESULT_VOID(ioWriteOpen(storageWriteIo(file)), "open file");
TEST_RESULT_INT(ioWriteFd(storageWriteIo(file)), -1, "check write fd");
TEST_RESULT_VOID(ioWriteClose(storageWriteIo(file)), "close file");
memContextFree(objMemContext((StorageSftp *)storageDriver(storageTest)));
// -------------------------------------------------------------------------------------------------------------------------
TEST_TITLE("ssh error on rename");