Changeset View
Standalone View
sftp/kio_sftp.cpp
Show First 20 Lines • Show All 133 Lines • ▼ Show 20 Line(s) | 133 | case ENOSPC: | |||
---|---|---|---|---|---|
134 | return ERR_DISK_FULL; | 134 | return ERR_DISK_FULL; | ||
135 | default: | 135 | default: | ||
136 | return ERR_CANNOT_WRITE; | 136 | return ERR_CANNOT_WRITE; | ||
137 | } | 137 | } | ||
138 | } | 138 | } | ||
139 | return 0; | 139 | return 0; | ||
140 | } | 140 | } | ||
141 | 141 | | |||
142 | static KIO::fileoffset_t seekPos(int fd, KIO::fileoffset_t pos, int mode) | | |||
143 | { | | |||
144 | KIO::fileoffset_t offset = -1; | | |||
145 | while ((offset = QT_LSEEK(fd, pos, mode)) == EAGAIN); | | |||
146 | return offset; | | |||
147 | } | | |||
148 | | ||||
149 | static bool wasUsernameChanged(const QString &username, const KIO::AuthInfo &info) | 142 | static bool wasUsernameChanged(const QString &username, const KIO::AuthInfo &info) | ||
150 | { | 143 | { | ||
151 | QString loginName (username); | 144 | QString loginName (username); | ||
152 | // If username is empty, assume the current logged in username. Why ? | 145 | // If username is empty, assume the current logged in username. Why ? | ||
153 | // Because libssh's SSH_OPTIONS_USER will default to that when it is not | 146 | // Because libssh's SSH_OPTIONS_USER will default to that when it is not | ||
154 | // set and it won't be set unless the user explicitly typed a user user | 147 | // set and it won't be set unless the user explicitly typed a user user | ||
155 | // name as part of the request URL. | 148 | // name as part of the request URL. | ||
156 | if (loginName.isEmpty()) { | 149 | if (loginName.isEmpty()) { | ||
▲ Show 20 Lines • Show All 1527 Lines • ▼ Show 20 Line(s) | 1676 | if (fd == -1) { | |||
1684 | // Maybe we can use this partial file for resuming | 1677 | // Maybe we can use this partial file for resuming | ||
1685 | // Tell about the size we have, and the app will tell us | 1678 | // Tell about the size we have, and the app will tell us | ||
1686 | // if it's ok to resume or not. | 1679 | // if it's ok to resume or not. | ||
1687 | qCDebug(KIO_SFTP_LOG) << "calling canResume with " << sbPart->size; | 1680 | qCDebug(KIO_SFTP_LOG) << "calling canResume with " << sbPart->size; | ||
1688 | flags |= q->canResume(sbPart->size) ? KIO::Resume : KIO::DefaultFlags; | 1681 | flags |= q->canResume(sbPart->size) ? KIO::Resume : KIO::DefaultFlags; | ||
1689 | qCDebug(KIO_SFTP_LOG) << "put got answer " << (flags & KIO::Resume); | 1682 | qCDebug(KIO_SFTP_LOG) << "put got answer " << (flags & KIO::Resume); | ||
1690 | 1683 | | |||
1691 | } else { | 1684 | } else { | ||
1692 | KIO::filesize_t pos = seekPos(fd, sbPart->size, SEEK_SET); | 1685 | KIO::filesize_t pos = QT_LSEEK(fd, sbPart->size, SEEK_SET); | ||
1693 | if (pos != sbPart->size) { | 1686 | if (pos != sbPart->size) { | ||
1694 | qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given" << strerror(errno); | 1687 | qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given:" << strerror(errno); | ||
bruns: You should save errno immediately after QT_LSEEK, otherwise any function may overwrite it. See… | |||||
Which call between the seek and the debug do you take offense here exactly? sitter: Which call between the seek and the debug do you take offense here exactly? | |||||
bruns: qCDebug(), operator<< | |||||
sitter: We do that all over the place. | |||||
bruns: Which part of
> A common mistake is to do
did you miss?
| |||||
Oh, I'm sorry. My reading comprehension isn't so good. I am not sure what you are asking of me though. errno access in qdebug streaming is used all over our software and this line here isn't even new. If the errno access wasn't a problem before, why is it now? And what does it have to do with the fix this diff has? Why don't you just do something about the errno problems? sitter: Oh, I'm sorry. My reading comprehension isn't so good. I am not sure what you are asking of me… | |||||
Essentially, apply the change from the man page example to the code here: if (pos != sbPart->size) { int errsv = errno; qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given:" << strerror(errsv); } As the errno value is now held in a local variable (and thus the global/thread-local errno does not matter), no function called from qCDebug() is able to mess with the value. qCDebug() *may* call all kind of functions (remember, it may log to the terminal, to the system journal, ...) which may fail in a non-fatal way ("if this does not work, try that"), overwriting errno in the course. Making assumptions about errno can lead to some surprises, and while it may work most of the time it is better to be save than sorry. bruns: Essentially, apply the change from the man page example to the code here:
```
if (pos !=… | |||||
Can you please make a diff for that? fish/fish.cpp: myDebug( << "socketpair failed, error: " << strerror(errno)); fish/fish.cpp: myDebug( << "fork failed, error: " << strerror(errno)); fish/fish.cpp: myDebug( << "could not exec! " << strerror(errno)); fish/fish.cpp: myDebug( << "select failed, rc: " << rc << ", error: " << strerror(errno)); fish/fish.cpp: myDebug( << "write failed, rc: " << rc << ", error: " << strerror(errno)); fish/fish.cpp: myDebug( << "read failed, rc: " << rc << ", error: " << strerror(errno)); fish/fish.cpp: myDebug( << "select failed, rc: " << rc << ", error: " << strerror(errno)); fish/fish.cpp: myDebug( << "write failed, rc: " << rc << ", error: " << strerror(errno)); fish/fish.cpp: myDebug( << "read failed, rc: " << rc << ", error: " << strerror(errno)); sftp/kio_sftp.cpp: qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << sbPart->size << "bytes in source file. Reason given" << strerror(errno); smb/kio_smb_dir.cpp: qCDebug(KIO_SMB) << "failed to rename" << dstUrl << "to" << dstOrigUrl << "->" << strerror(errno); smb/kio_smb_browse.cpp: return SMBError{ ERR_INTERNAL, i18n("Unknown error condition in stat: %1", QString::fromLocal8Bit( strerror(errNum))) }; sitter: Can **you** please make a diff for that?
```
fish/fish.cpp: myDebug( << "socketpair… | |||||
1695 | sftp_attributes_free(sb); | 1688 | sftp_attributes_free(sb); | ||
1696 | sftp_attributes_free(sbPart); | 1689 | sftp_attributes_free(sbPart); | ||
1697 | return Result::fail(ERR_CANNOT_SEEK, url.toString()); | 1690 | return Result::fail(ERR_CANNOT_SEEK, url.toString()); | ||
1698 | } | 1691 | } | ||
1699 | flags |= KIO::Resume; | 1692 | flags |= KIO::Resume; | ||
1700 | } | 1693 | } | ||
1701 | qCDebug(KIO_SFTP_LOG) << "Resuming at" << sbPart->size; | 1694 | qCDebug(KIO_SFTP_LOG) << "Resuming at" << sbPart->size; | ||
1702 | sftp_attributes_free(sbPart); | 1695 | sftp_attributes_free(sbPart); | ||
▲ Show 20 Lines • Show All 207 Lines • ▼ Show 20 Line(s) | |||||
1910 | 1903 | | |||
1911 | Result SFTPInternal::sftpCopyGet(const QUrl &url, const QString &sCopyFile, int permissions, KIO::JobFlags flags) | 1904 | Result SFTPInternal::sftpCopyGet(const QUrl &url, const QString &sCopyFile, int permissions, KIO::JobFlags flags) | ||
1912 | { | 1905 | { | ||
1913 | qCDebug(KIO_SFTP_LOG) << url << "->" << sCopyFile << ", permissions=" << permissions; | 1906 | qCDebug(KIO_SFTP_LOG) << url << "->" << sCopyFile << ", permissions=" << permissions; | ||
1914 | 1907 | | |||
1915 | // check if destination is ok ... | 1908 | // check if destination is ok ... | ||
1916 | QFileInfo copyFile(sCopyFile); | 1909 | QFileInfo copyFile(sCopyFile); | ||
1917 | const bool bDestExists = copyFile.exists(); | 1910 | const bool bDestExists = copyFile.exists(); | ||
1918 | if (bDestExists) { | 1911 | if (bDestExists) { | ||
1919 | if (copyFile.isDir()) { | 1912 | if (copyFile.isDir()) { | ||
1920 | return Result::fail(ERR_IS_DIRECTORY, sCopyFile); | 1913 | return Result::fail(ERR_IS_DIRECTORY, sCopyFile); | ||
1921 | } | 1914 | } | ||
1922 | 1915 | | |||
1923 | if (!(flags & KIO::Overwrite)) { | 1916 | if (!(flags & KIO::Overwrite)) { | ||
1924 | return Result::fail(ERR_FILE_ALREADY_EXIST, sCopyFile); | 1917 | return Result::fail(ERR_FILE_ALREADY_EXIST, sCopyFile); | ||
1925 | } | 1918 | } | ||
1926 | } | 1919 | } | ||
Show All 27 Lines | 1946 | #endif | |||
1954 | else | 1947 | else | ||
1955 | initialMode = 0666; | 1948 | initialMode = 0666; | ||
1956 | 1949 | | |||
1957 | // open the output file ... | 1950 | // open the output file ... | ||
1958 | int fd = -1; | 1951 | int fd = -1; | ||
1959 | KIO::fileoffset_t offset = 0; | 1952 | KIO::fileoffset_t offset = 0; | ||
1960 | if (bResume) { | 1953 | if (bResume) { | ||
1961 | fd = QT_OPEN( QFile::encodeName(sPart), O_RDWR ); // append if resuming | 1954 | fd = QT_OPEN( QFile::encodeName(sPart), O_RDWR ); // append if resuming | ||
1962 | offset = seekPos(fd, 0, SEEK_END); | 1955 | offset = QT_LSEEK(fd, partFile.size(), SEEK_SET); | ||
1963 | if(offset < 0) { | 1956 | if (offset != partFile.size()) { | ||
1957 | qCDebug(KIO_SFTP_LOG) << "Failed to seek to" << partFile.size() << "bytes in target file. Reason given:" << strerror(errno); | ||||
bruns: dito | |||||
1964 | ::close(fd); | 1958 | ::close(fd); | ||
1965 | return Result::fail(ERR_CANNOT_RESUME, sCopyFile); | 1959 | return Result::fail(ERR_CANNOT_RESUME, sCopyFile); | ||
1966 | } | 1960 | } | ||
1967 | qCDebug(KIO_SFTP_LOG) << "resuming at" << offset; | 1961 | qCDebug(KIO_SFTP_LOG) << "resuming at" << offset; | ||
1968 | } | 1962 | } | ||
1969 | else { | 1963 | else { | ||
1970 | fd = QT_OPEN(QFile::encodeName(dest), O_CREAT | O_TRUNC | O_WRONLY, initialMode); | 1964 | fd = QT_OPEN(QFile::encodeName(dest), O_CREAT | O_TRUNC | O_WRONLY, initialMode); | ||
1971 | } | 1965 | } | ||
▲ Show 20 Lines • Show All 772 Lines • Show Last 20 Lines |
You should save errno immediately after QT_LSEEK, otherwise any function may overwrite it. See man 3 errno: