Changeset View
Standalone View
src/core/copyjob.cpp
Show All 13 Lines | 1 | /* This file is part of the KDE libraries | |||
---|---|---|---|---|---|
14 | Library General Public License for more details. | 14 | Library General Public License for more details. | ||
15 | 15 | | |||
16 | You should have received a copy of the GNU Library General Public License | 16 | You should have received a copy of the GNU Library General Public License | ||
17 | along with this library; see the file COPYING.LIB. If not, write to | 17 | along with this library; see the file COPYING.LIB. If not, write to | ||
18 | the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, | 18 | the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, | ||
19 | Boston, MA 02110-1301, USA. | 19 | Boston, MA 02110-1301, USA. | ||
20 | */ | 20 | */ | ||
21 | 21 | | |||
22 | #include <config-kiocore.h> | ||||
23 | | ||||
22 | #include "copyjob.h" | 24 | #include "copyjob.h" | ||
23 | #include "kiocoredebug.h" | 25 | #include "kiocoredebug.h" | ||
24 | #include <errno.h> | 26 | #include <errno.h> | ||
27 | #if HAVE_SYS_XATTR_H | ||||
28 | #if defined(Q_OS_LINUX) || defined(__GLIBC__) || defined(Q_OS_MAC) | ||||
29 | #include <sys/xattr.h> | ||||
30 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
31 | #include <sys/extattr.h> | ||||
32 | #endif | ||||
33 | #endif | ||||
pino: this `#include` block has a slightly messy logic:
- if `sys/attr.h` is available, just include… | |||||
25 | #include "kcoredirlister.h" | 34 | #include "kcoredirlister.h" | ||
26 | #include "kfileitem.h" | 35 | #include "kfileitem.h" | ||
27 | #include "job.h" // buildErrorString | 36 | #include "job.h" // buildErrorString | ||
28 | #include "mkdirjob.h" | 37 | #include "mkdirjob.h" | ||
29 | #include "listjob.h" | 38 | #include "listjob.h" | ||
30 | #include "statjob.h" | 39 | #include "statjob.h" | ||
31 | #include "deletejob.h" | 40 | #include "deletejob.h" | ||
32 | #include "filecopyjob.h" | 41 | #include "filecopyjob.h" | ||
▲ Show 20 Lines • Show All 196 Lines • ▼ Show 20 Line(s) | 236 | // KIO::Job* linkNextFile( const QUrl& uSource, const QUrl& uDest, bool overwrite ); | |||
229 | void copyNextFile(); | 238 | void copyNextFile(); | ||
230 | void slotResultDeletingDirs(KJob *job); | 239 | void slotResultDeletingDirs(KJob *job); | ||
231 | void deleteNextDir(); | 240 | void deleteNextDir(); | ||
232 | void sourceStated(const UDSEntry &entry, const QUrl &sourceUrl); | 241 | void sourceStated(const UDSEntry &entry, const QUrl &sourceUrl); | ||
233 | void skip(const QUrl &sourceURL, bool isDir); | 242 | void skip(const QUrl &sourceURL, bool isDir); | ||
234 | void slotResultRenaming(KJob *job); | 243 | void slotResultRenaming(KJob *job); | ||
235 | void slotResultSettingDirAttributes(KJob *job); | 244 | void slotResultSettingDirAttributes(KJob *job); | ||
236 | void setNextDirAttribute(); | 245 | void setNextDirAttribute(); | ||
246 | void copyXattrs(const QUrl &source, const QUrl &dest); | ||||
237 | 247 | | |||
238 | void startRenameJob(const QUrl &slave_url); | 248 | void startRenameJob(const QUrl &slave_url); | ||
239 | bool shouldOverwriteDir(const QString &path) const; | 249 | bool shouldOverwriteDir(const QString &path) const; | ||
240 | bool shouldOverwriteFile(const QString &path) const; | 250 | bool shouldOverwriteFile(const QString &path) const; | ||
241 | bool shouldSkip(const QString &path) const; | 251 | bool shouldSkip(const QString &path) const; | ||
242 | void skipSrc(bool isDir); | 252 | void skipSrc(bool isDir); | ||
243 | void renameDirectory(const QList<CopyInfo>::iterator &it, const QUrl &newUrl); | 253 | void renameDirectory(const QList<CopyInfo>::iterator &it, const QUrl &newUrl); | ||
244 | QUrl finalDestUrl(const QUrl &src, const QUrl &dest) const; | 254 | QUrl finalDestUrl(const QUrl &src, const QUrl &dest) const; | ||
▲ Show 20 Lines • Show All 865 Lines • ▼ Show 20 Line(s) | 1074 | if ((m_conflictError == ERR_DIR_ALREADY_EXIST) | |||
1110 | } | 1120 | } | ||
1111 | } else { | 1121 | } else { | ||
1112 | // Severe error, abort | 1122 | // Severe error, abort | ||
1113 | q->Job::slotResult(job); // will set the error and emit result(this) | 1123 | q->Job::slotResult(job); // will set the error and emit result(this) | ||
1114 | return; | 1124 | return; | ||
1115 | } | 1125 | } | ||
1116 | } else { // no error : remove from list, to move on to next dir | 1126 | } else { // no error : remove from list, to move on to next dir | ||
1117 | //this is required for the undo feature | 1127 | //this is required for the undo feature | ||
1128 | copyXattrs((*it).uSource, (*it).uDest); | ||||
1118 | emit q->copyingDone(q, (*it).uSource, finalDestUrl((*it).uSource, (*it).uDest), (*it).mtime, true, false); | 1129 | emit q->copyingDone(q, (*it).uSource, finalDestUrl((*it).uSource, (*it).uDest), (*it).mtime, true, false); | ||
cfeck: Here, too. | |||||
I tried various ways to call this as a subjob, but all of them leads to breakage of non xattr related tests. Maybe some major changes to the class are needed. But the call can be asynchronous with little effort. All tests still pass if start() is called, instead of exec(). cochise: I tried various ways to call this as a subjob, but all of them leads to breakage of non xattr… | |||||
That sounds racy; the subjob might not be finished by the time the main job is finished, if you just "start and forget". I cannot accept this commit with an exec() in here. The easy and modern way to make this async is just a connect and a lambda (which contains the rest of the code here). I have to wonder though: do we have a fast way to determine whether we actually need the additional subjob? (to avoid making this slower on systems -- or files -- without xattr) Hmm, or a much bigger architectural question: why doesn't kio_file copy the xattr as part of FileProtocol::copy(), as it already does with e.g. permissions and ACLs, instead of doing this in a separate job? dfaure: That sounds racy; the subjob might not be finished by the time the main job is finished, if you… | |||||
Please help me understand for a moment. I'm not the original author of this patch and am trying to bring it to completion. I just need to be caught up to speed. In reading this piece of code over, I wasn't entirely sure of the author's intent. It seems like he wants to create a new job which copies the xattrs and then run it asynchronously. Why is the exec call bad? What does a lambda do that the exec call does not? In terms of determining when the job is actually required, one could test to see if the file has xattrs or indeed if the system has xattrs support. We could surround the invokation of the xattrs copy job with an #ifdef HAVE_SYS_XATTR_H, for example. tmarshall: Please help me understand for a moment. I'm not the original author of this patch and am trying… | |||||
tmarshall: And do we want this to be sync or async? That much isn't clear to me. | |||||
Thanks to @ahmad78 for chatting with me about this in irc. Everything works for me if I replace the exec call with a start call. Currently the result of the exec call is thrown away as the job itself does the error handling and there isn't much to do if the xattrs can't be copied for whatever reason. As for why this is a different job, that was a design decision by the original author. It seems like a decent enough way forward to me as it leans on the side of modularity. The applications of xattrs are so broad that it's hard to say what they will be used for in the future or what the desired functionality might be. Having a separate job will allow for more flexibility regarding xattrs going forward. In short, I don't really think it hurts to have a separate job and there is certainly the potential that it comes in handy. tmarshall: Thanks to @ahmad78 for chatting with me about this in irc.
Everything works for me if I… | |||||
I don't mind that errors don't propagate up here, that's fine, we certainly don't want the copy to fail if attributes can't be copied. I do mind that this uses exec() (Qt eventloop re-entrancy is a nasty source of bugs), and I do mind a fire-and-forget subjob (if that's what start() here does -- unless the subjob gets registered?). I have provided the solution already, connect the subjob's result to a lambda that contains the rest of the code in this method [but see below]. I just realized another problem: this is at the wrong layer. It should be in FileCopyJob, not in the high-level CopyJob (which lists directories, creates directories, etc. and delegates the task of copying one file to FileCopyJob). FileCopyJob is also used directly in many places (when a single file has to be copied) so if you want that to preserve xattr (rethorical question, I'm assuming you do) this should go down to FileCopyJob. About this being a different job: actually, one doesn't prevent the other. Look at permissions. We have ChmodJob for setting permissions, but when FileCopyJob finds that the kioslave supports copy() (see DirectCopyJob on the app side) then copy() takes care of copying permissions too. FileCopyJob only calls ChmodJob for the special case of renaming and because the FileCopyJob API actually allows to change permissions, this doesn't apply to xattr. When the kioslave doesn't support copy() and it falls back to "data pump" mode (get() + put()), FileCopyJob passes the permissions to the put() job. *That* is the case where it should follow that with a KIO::copy_xattr subjob, unless xattr can be passed by metadata to the put job? I believe the same idea should be done for xattr (kio_file's copy() should copy xattr all by itself), to minimize roundtrips between the app and the slave. People complain that copying files with KIO is too slow, let's not make it worse. dfaure: I don't mind that errors don't propagate up here, that's fine, we certainly don't want the copy… | |||||
1119 | m_directoriesCopied.append(*it); | 1130 | m_directoriesCopied.append(*it); | ||
1120 | dirs.erase(it); | 1131 | dirs.erase(it); | ||
1121 | } | 1132 | } | ||
1122 | 1133 | | |||
1123 | m_processedDirs++; | 1134 | m_processedDirs++; | ||
1124 | //emit processedAmount( this, KJob::Directories, m_processedDirs ); | 1135 | //emit processedAmount( this, KJob::Directories, m_processedDirs ); | ||
1125 | q->removeSubjob(job); | 1136 | q->removeSubjob(job); | ||
1126 | Q_ASSERT(!q->hasSubjobs()); // We should have only one job at a time ... | 1137 | Q_ASSERT(!q->hasSubjobs()); // We should have only one job at a time ... | ||
▲ Show 20 Lines • Show All 226 Lines • ▼ Show 20 Line(s) | 1349 | } else { // no error | |||
1353 | const QUrl finalUrl = finalDestUrl((*it).uSource, (*it).uDest); | 1364 | const QUrl finalUrl = finalDestUrl((*it).uSource, (*it).uDest); | ||
1354 | 1365 | | |||
1355 | if (m_bCurrentOperationIsLink) { | 1366 | if (m_bCurrentOperationIsLink) { | ||
1356 | QString target = (m_mode == CopyJob::Link ? (*it).uSource.path() : (*it).linkDest); | 1367 | QString target = (m_mode == CopyJob::Link ? (*it).uSource.path() : (*it).linkDest); | ||
1357 | //required for the undo feature | 1368 | //required for the undo feature | ||
1358 | emit q->copyingLinkDone(q, (*it).uSource, target, finalUrl); | 1369 | emit q->copyingLinkDone(q, (*it).uSource, target, finalUrl); | ||
1359 | } else { | 1370 | } else { | ||
1360 | //required for the undo feature | 1371 | //required for the undo feature | ||
1372 | copyXattrs((*it).uSource, (*it).uDest); | ||||
1361 | emit q->copyingDone(q, (*it).uSource, finalUrl, (*it).mtime, false, false); | 1373 | emit q->copyingDone(q, (*it).uSource, finalUrl, (*it).mtime, false, false); | ||
1362 | if (m_mode == CopyJob::Move) { | 1374 | if (m_mode == CopyJob::Move) { | ||
1363 | org::kde::KDirNotify::emitFileMoved((*it).uSource, finalUrl); | 1375 | org::kde::KDirNotify::emitFileMoved((*it).uSource, finalUrl); | ||
1364 | } | 1376 | } | ||
1365 | m_successSrcList.append((*it).uSource); | 1377 | m_successSrcList.append((*it).uSource); | ||
1366 | if (m_freeSpace != (KIO::filesize_t) - 1 && (*it).size != (KIO::filesize_t) - 1) { | 1378 | if (m_freeSpace != (KIO::filesize_t) - 1 && (*it).size != (KIO::filesize_t) - 1) { | ||
1367 | m_freeSpace -= (*it).size; | 1379 | m_freeSpace -= (*it).size; | ||
1368 | } | 1380 | } | ||
▲ Show 20 Lines • Show All 799 Lines • ▼ Show 20 Line(s) | 2136 | { | |||
2168 | case STATE_SETTING_DIR_ATTRIBUTES: | 2180 | case STATE_SETTING_DIR_ATTRIBUTES: | ||
2169 | d->slotResultSettingDirAttributes(job); | 2181 | d->slotResultSettingDirAttributes(job); | ||
2170 | break; | 2182 | break; | ||
2171 | default: | 2183 | default: | ||
2172 | Q_ASSERT(0); | 2184 | Q_ASSERT(0); | ||
2173 | } | 2185 | } | ||
2174 | } | 2186 | } | ||
2175 | 2187 | | |||
2188 | // copy xattr to new dir | ||||
2189 | void CopyJobPrivate::copyXattrs(const QUrl &source, const QUrl &dest) | ||||
2190 | { | ||||
2191 | #if !(HAVE_SYS_XATTR_H) | ||||
2192 | return; | ||||
2193 | #endif | ||||
if HAVE_SYS_XATTR_H is not defined, the first instruction in copyXattrs will be a return, and some compilers may warn/error out because the rest of the function is unreachable; better stub out the whole function instead pino: if `HAVE_SYS_XATTR_H` is not defined, the first instruction in `copyXattrs` will be a return… | |||||
I think looks better this way, but if it throw a warning I will revert to encapsulating the whole function. cochise: I think looks better this way, but if it throw a warning I will revert to encapsulating the… | |||||
2194 | long listlen, valuelen, destlen; | ||||
pino: destiny is another thing ;) "destination" in this case | |||||
2195 | const QByteArray sourcearray = source.path().toLocal8Bit().data(); | ||||
pino: - you don't need to call `data()` here, the return value of `toLocal8Bit()` is already a… | |||||
I need data(), because the compiler complains about non private cast. but using encodeName() should be better either way. cochise: I need `data()`, because the compiler complains about non private cast. but using `encodeName… | |||||
2196 | const char *xattrsrc = sourcearray.data(); | ||||
2197 | // get size of key list | ||||
2198 | #if defined(Q_OS_LINUX) || (defined(__GLIBC__) && !defined(__stub_getxattr)) | ||||
2199 | listlen = listxattr(xattrsrc, nullptr, 0); | ||||
2200 | #elif defined(Q_OS_MAC) | ||||
2201 | listlen = listxattr(xattrsrc, NULL, 0, 0); | ||||
pino: NULL -> nullptr | |||||
you can (typically) avoid the double (syscall, i.e. expensive) by preallocating the array and only reallocating if you get errno == ERANGE. Dito for getxattr. bruns: you can (typically) avoid the double (syscall, i.e. expensive) by preallocating the array and… | |||||
In theory, xattrs have unlimited size on some filesystems. Ext limits it to a fs block (4 Kb on most end user setups). Allocating full 4 kb is a overkill, as most of xattr ae small. But as most of files don't have xattr, and the function will return here, we have to pay the cost of the second call only if we will use it. If we preallocate memory, we have to pay a cost for every file. cochise: In theory, xattrs have unlimited size on some filesystems. Ext limits it to a fs block (4 Kb on… | |||||
2202 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
2203 | listlen = extattr_list_file(xattr_src, EXTATTR_NAMESPACE_USER, NULL, 0); | ||||
pino: NULL -> nullptr | |||||
2204 | #endif | ||||
2205 | if (listlen < 0) { | ||||
2206 | qCWarning(KIO_COPYJOB_DEBUG) << "Glibc failed to extract xattr."; | ||||
there is not just glibc; also, better check for errno as ENOTSUP, because that means the source filesystem does not support xattrs, and thus you can directly skip the rest of the function (as it will not work anyway) pino: there is not just glibc; also, better check for `errno` as `ENOTSUP`, because that means the… | |||||
If a error is found at this point, all the rest of the function will not run, because it is on the a statement, but doing a early check for destination filesystem support of xattr is a good idea. cochise: If a error is found at this point, all the rest of the function will not run, because it is on… | |||||
2207 | } else if (listlen == 0) { | ||||
2208 | qCDebug(KIO_COPYJOB_DEBUG) << "File don't have any xattr."; | ||||
2209 | } else { | ||||
2210 | QByteArray keylist(listlen, Qt::Uninitialized); | ||||
2211 | // get the key list | ||||
this is more of a personal taste rather than an issue: IMHO you can avoid the "if .. else if ... else" that adds a nesting level more, especially when all the checks lead to early return; so something like: if (listlen < 0) { if (errno != ENOTSUP) { qCWarning(KIO_COPYJOB_DEBUG) << "failed to extract xattr from" << xattrsrc; } return; } if (listlen == 0) { qCDebug(KIO_COPYJOB_DEBUG) << "File" << xattrsrc << "doesn't have any xattr."; return; } [etc..] pino: this is more of a personal taste rather than an issue: IMHO you can avoid the "if .. else if ... | |||||
2212 | #if defined(Q_OS_LINUX) || (defined(__GLIBC__) && !defined(__stub_getxattr)) | ||||
2213 | listlen = listxattr(xattrsrc, keylist.data(), listlen); | ||||
2214 | #elif defined(Q_OS_MAC) | ||||
2215 | listlen = listxattr(xattrsrc, keylist.data(), listlen, 0); | ||||
2216 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
2217 | listlen = extattr_list_file(xattrsrc, EXTATTR_NAMESPACE_USER, keylist.data(), listlen); | ||||
2218 | #endif | ||||
2219 | qCWarning(KIO_COPYJOB_DEBUG) << listlen << xattrsrc << keylist.data(); | ||||
2220 | QList<QByteArray> xattrkeys = keylist.split('\0'); | ||||
2221 | xattrkeys.removeLast(); // the last item is alwys empty | ||||
2222 | for (const auto & xattrkey : xattrkeys) { | ||||
2223 | // get the size of value for key | ||||
bruns: `... always ...` | |||||
On Linux, at least. Each item gets a \0 terminator and the list itself too. Not tested on other systems. cochise: On Linux, at least. Each item gets a `\0` terminator and the list itself too. Not tested on… | |||||
2224 | #if defined(Q_OS_LINUX) || (defined(__GLIBC__) && !defined(__stub_getxattr)) | ||||
2225 | valuelen = getxattr(xattrsrc, xattrkey.data(), nullptr, 0); | ||||
2226 | #elif defined(Q_OS_MAC) | ||||
2227 | valuelen = listxattr(xattrsrc, xattrkey.data(), NULL, 0, 0, 0); | ||||
pino: NULL -> nullptr | |||||
bruns: There should probably be a `if (!xattrkey.startsWith("user.")) continue;` here. | |||||
Doing some research and test...
If a copy a file with kioclient as user, any attribute outside user. is lost and I get a warning for the ones I'm unable to preserve, but if I copy as root, all are preserved. cochise: Doing some research and test...
> Currently, the security, system, trusted, and user extended… | |||||
2228 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
getxattr is called on the same file again and again, thus it is much more efficient to use fgetxattr. bruns: getxattr is called on the same file again and again, thus it is much more efficient to use… | |||||
2229 | valuelen = extattr_get_file(xattrsrc, EXTATTR_NAMESPACE_USER, xattrkey.data(), NULL, 0); | ||||
pino: NULL -> nullptr | |||||
2230 | #endif | ||||
2231 | if (valuelen < 1) { | ||||
2232 | qCWarning(KIO_COPYJOB_DEBUG) << "Glibc failed to extract value for a xattr key."; | ||||
2233 | continue; | ||||
2234 | } | ||||
2235 | if (valuelen == 0) { | ||||
2236 | qCDebug(KIO_COPYJOB_DEBUG) << "empty xattr key."; // They are legal, but... Baloo don't use it | ||||
2237 | } | ||||
2238 | QByteArray xattrval(valuelen + 1, Qt::Uninitialized); | ||||
2239 | //get the value of the key | ||||
2240 | #if defined(Q_OS_LINUX) || (defined(__GLIBC__) && !defined(__stub_getxattr)) | ||||
2241 | valuelen = getxattr(xattrsrc, xattrkey.data(), xattrval.data(), valuelen); | ||||
2242 | #elif defined(Q_OS_MAC) | ||||
2243 | vallen = getxattr(xattr_src, xattrkey.data(), xattrkey.data(), xattrval.data(), valuelen, 0, 0); | ||||
2244 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
2245 | vallen = extattr_get_file(xattr_src, EXTATTR_NAMESPACE_USER, xattrkey.data(), xattrval.data(), valuelen); | ||||
2246 | #endif | ||||
2247 | //write key:value pair on dest file | ||||
2248 | const QByteArray destarray = source.path().toLocal8Bit().data(); | ||||
2249 | const char *xattrdest = sourcearray.data(); | ||||
2250 | #if defined(Q_OS_LINUX) || (defined(__GLIBC__) && !defined(__stub_getxattr)) | ||||
2251 | destlen = setxattr(xattrdest, xattrkey.data(), xattrval.data(), valuelen, 0); | ||||
2252 | #elif defined(Q_OS_MAC) | ||||
2253 | destlen = setxattr(xattrdest, xattrkey.data(), xattrval.data(), valuelen, 0, 0); | ||||
2254 | #elif defined(Q_OS_FREEBSD) || defined(Q_OS_NETBSD) | ||||
2255 | destlen = extattr_set_file(xattrdest, EXTATTR_NAMESPACE_USER, xattr_key.data(), xattrval.data(), valuelen); | ||||
2256 | #endif | ||||
2257 | if (destlen < 0) { | ||||
2258 | qCWarning(KIO_COPYJOB_DEBUG) << "Glibc failed to write xattr on a file."; | ||||
pino: IMHO this is not a warning worth situation | |||||
2259 | } | ||||
as noted above: checking that errno is ENOTSUP means that the destination filesystem does not support xattrs, so there is little point trying to set the other attributes pino: as noted above: checking that `errno` is `ENOTSUP` means that the destination filesystem does… | |||||
2260 | } | ||||
2261 | } | ||||
2262 | } | ||||
2263 | | ||||
2176 | void KIO::CopyJob::setDefaultPermissions(bool b) | 2264 | void KIO::CopyJob::setDefaultPermissions(bool b) | ||
2177 | { | 2265 | { | ||
2178 | d_func()->m_defaultPermissions = b; | 2266 | d_func()->m_defaultPermissions = b; | ||
2179 | } | 2267 | } | ||
2180 | 2268 | | |||
2181 | KIO::CopyJob::CopyMode KIO::CopyJob::operationMode() const | 2269 | KIO::CopyJob::CopyMode KIO::CopyJob::operationMode() const | ||
2182 | { | 2270 | { | ||
2183 | return d_func()->m_mode; | 2271 | return d_func()->m_mode; | ||
▲ Show 20 Lines • Show All 107 Lines • Show Last 20 Lines |
this #include block has a slightly messy logic: