Differential D15180 Diff 40888 src/kioexec/kioexecd.cpp

Changeset View

Standalone View

View Options

src/kioexec/kioexecd.cpp

Show All 27 Lines
28#include <KPluginFactory> 28​#include <KPluginFactory>
29#include <KMessageBox> 29​#include <KMessageBox>
30 30​
31#include <QDir> 31​#include <QDir>
32#include <QFileInfo> 32​#include <QFileInfo>
33#include <QStandardPaths> 33​#include <QStandardPaths>
34#include <QUrl> 34​#include <QUrl>
35 35​
36K_PLUGIN_FACTORY_WITH_JSON(KIOExecdFactory, 36​K_PLUGIN_FACTORY_WITH_JSON(KIOExecdFactory,
dfaureUnsubmitted
Not Done

static const int...

dfaure: static const int...
37 "kioexecd.json", 37​ "kioexecd.json",
38 registerPlugin<KIOExecd>();) 38​ registerPlugin<KIOExecd>();)
39 39​
40KIOExecd::KIOExecd(QObject *parent, const QList<QVariant> &) 40​KIOExecd::KIOExecd(QObject *parent, const QList<QVariant> &)
41 : KDEDModule(parent) 41​ : KDEDModule(parent)
42{ 42​{
43 qCDebug(KIOEXEC) << "kioexecd started"; 43​ qCDebug(KIOEXEC) << "kioexecd started";
44 44​
45 new KIOExecdAdaptor(this); 45​ new KIOExecdAdaptor(this);
46 m_watcher = new KDirWatch(this); 46​ m_watcher = new KDirWatch(this);
47 47​
48 connect(m_watcher, &KDirWatch::dirty, this, &KIOExecd::slotDirty); 48​ connect(m_watcher, &KDirWatch::dirty, this, &KIOExecd::slotDirty);
49 connect(m_watcher, &KDirWatch::deleted, this, &KIOExecd::slotDeleted); 49​ connect(m_watcher, &KDirWatch::created, this, &KIOExecd::slotDirty);
50} 50​}
anthonyfieroniUnsubmitted
Done

Also add interval here, setInterval, in other place just start()

anthonyfieroni: Also add interval here, setInterval, in other place just start()
51 51​
52KIOExecd::~KIOExecd() 52​KIOExecd::~KIOExecd()
53{ 53​{
54​ // Remove the remaining temporary directories with all their files.
54 for (auto it = m_watched.constBegin(); it != m_watched.constEnd(); ++it) { 55​ for (auto it = m_watched.constBegin(); it != m_watched.constEnd(); ++it) {
55 QFileInfo info(it.key()); 56​ QFileInfo info(it.key());
56 const auto parentDir = info.path(); 57​ const auto parentDir = info.path();
57 qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" << info.fileName(); 58​ qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" << info.fileName();
58 QFile::remove(it.key()); 59​ QDir(parentDir).removeRecursively();
broulikUnsubmitted
Done

I'm not sure that's a good idea

broulik: I'm not sure that's a good idea
jtamateAuthorUnsubmitted
Not Done

Why not?
It should be just removing the temporary file copy and it's backups.

jtamate: Why not? It should be just removing the temporary file copy and it's backups.
broulikUnsubmitted
Done

If you can be sure kioexec creates a folder per temp file (it might does), then this is probably fine.
Also check kioexec what it does when the process closes, if it also recursively removes the temp dir

broulik: If you can be sure kioexec creates a folder per temp file (it might does), then this is…
jtamateAuthorUnsubmitted
Not Done

kioexec creates a folder per temp file.
kioexec doesn't remove recursively, therefore the temporary directory will not be deleted if the program creates backup files, like a.txt~

jtamate: kioexec creates a folder per temp file. kioexec doesn't remove recursively, therefore the…
elvisangelaccioUnsubmitted
Not Done

The problem with using QDir::removeRecursively() is that the folder we are going to delete recursively is an input from dbus. What happens if some malicious software calls watch("~/dummy.txt") ?

At the very least we need to check whether this folder starts with QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + QStringLiteral("/krun") (the path used by kioexec).

elvisangelaccio: The problem with using `QDir::removeRecursively()` is that the folder we are going to delete…
jtamateAuthorUnsubmitted
Not Done

There is a slightly problem: QStandardPaths::CacheLocation is application dependent, and their values doesn't match here:
kioexec: /home/jtorres/.cache/kioexec/
kioexecd: /home/jtorres/.cache/kiod5/
Can we assume that replacing kiod5 by kioexec will always work?

We could use QStandardPaths::GenericCacheLocation instead, but this is not guaranteed to be non empty.

Or another solution: keep it as it was (delete only the file and the directory if it is possible).

jtamate: There is a slightly problem: QStandardPaths::CacheLocation is application dependent, and their…
anthonyfieroniUnsubmitted
Not Done

You can remove only file and then if dir is empty to delete it. Same in slotCheckDeletedFiles

anthonyfieroni: You can remove only file and then if dir is empty to delete it. Same in slotCheckDeletedFiles
59 QDir().rmdir(parentDir); ​
60 } 60​ }
61} 61​}
62 62​
63void KIOExecd::watch(const QString &path, const QString &destUrl) 63​void KIOExecd::watch(const QString &path, const QString &destUrl)
64{ 64​{
65 if (m_watched.contains(path)) { 65​ if (m_watched.contains(path)) {
66 qCDebug(KIOEXEC) << "Already watching" << path; 66​ qCDebug(KIOEXEC) << "Already watching" << path;
67 return; 67​ return;
68 } 68​ }
69 69​
70 qCDebug(KIOEXEC) << "Going to watch" << path << "for changes, remote destination is" << destUrl; 70​ qCDebug(KIOEXEC) << "Going to watch" << path << "for changes, remote destination is" << destUrl;
71 71​
72​ // Watch the temporary file for modifications or creations
73​ m_watcher->addFile(path);
74​ m_watched.insert(path, QUrl(destUrl));
75​}
76​
77​void KIOExecd::watchCommand(const QString &path, const QString &destUrl, const QString &command)
78​{
79​ m_openedBy.insertMulti(command, path);
80​ m_appCounter.insertMulti(command, 1);
81​
82​ if (m_watched.contains(path)) {
83​ qCDebug(KIOEXEC) << "Already watching" << path;
84​ return;
85​ }
86​
87​ qCDebug(KIOEXEC) << "Going to watch" << path << "for changes, remote destination is" << destUrl;
88​
89​ // Watch the temporary file for modifications or creations
72 m_watcher->addFile(path); 90​ m_watcher->addFile(path);
73 m_watched.insert(path, QUrl(destUrl)); 91​ m_watched.insert(path, QUrl(destUrl));
74} 92​}
broulikUnsubmitted
Done

You never unwatch the dir

broulik: You never unwatch the dir
jtamateAuthorUnsubmitted
Not Done

No, only at the destructor, unless there is a way to know when the program that uses the file has been closed, I guess keeping the file during all the session is the same behavior as before.

jtamate: No, only at the destructor, unless there is a way to know when the program that uses the file…
broulikUnsubmitted
Not Done

I see. Previously it would just remove the watcher when the temp file is removed. Not sure how to fix it now, maybe we need an unwatch dbus call.
Imho we shouldn't change the behavior here too much, ie. "randomly" watching the parent dir instead of the actual file passed in as argument but instead introduce a watchDirectory dbus call that exhibits the new behavior? Not sure.

broulik: I see. Previously it would just remove the watcher when the temp file is removed. Not sure how…
jtamateAuthorUnsubmitted
Not Done

The watch will be left as it was, introducing the create case and removing the deleted case.

I've implemented an unwatch dbus call.

jtamate: The watch will be left as it was, introducing the create case and removing the deleted case.
75 93​
76void KIOExecd::slotDirty(const QString &path) 94​void KIOExecd::slotDirty(const QString &path)
77{ 95​{
78 if (!m_watched.contains(path)) { 96​ if (!m_watched.contains(path)) {
anthonyfieroniUnsubmitted
Done

Contains should be also in the guard.

anthonyfieroni: Contains should be also in the guard.
dfaureUnsubmitted
Not Done

Why is there a mutex at all, in this single-threaded code? This doesn't make sense to me.

dfaure: Why is there a mutex at all, in this single-threaded code? This doesn't make sense to me.
79 return; 97​ return;
80 } 98​ }
dfaureUnsubmitted
Not Done

Doesn't kdirwatch emit dirty when emitting created anyway?
(because historically, there was only "dirty" initially)

But I could be confusing watching files and watching directories, so better test rather than trusting me ;)

dfaure: Doesn't kdirwatch emit dirty when emitting created anyway? (because historically, there was…
81 99​
82 const auto dest = m_watched.value(path); 100​ const auto dest = m_watched.value(path);
anthonyfieroniUnsubmitted
Done

Now it's not needed 'remove' will do the work

anthonyfieroni: Now it's not needed 'remove' will do the work
jtamateAuthorUnsubmitted
Not Done

You're right. In this case it isn't possible to be notified of a file creation unless it has been deleted first.

jtamate: You're right. In this case it isn't possible to be notified of a file creation unless it has…
83 101​
84 if (KMessageBox::questionYesNo(nullptr, 102​ if (KMessageBox::questionYesNo(nullptr,
85 i18n("The file %1\nhas been modified. Do you want to upload the changes?" , dest.toDisplayString()), 103​ i18n("The file %1\nhas been modified. Do you want to upload the changes?" , dest.toDisplayString()),
86 i18n("File Changed"), KGuiItem(i18n("Upload")), KGuiItem(i18n("Do Not Upload"))) != KMessageBox::Yes) { 104​ i18n("File Changed"), KGuiItem(i18n("Upload")), KGuiItem(i18n("Do Not Upload"))) != KMessageBox::Yes) {
87 return; 105​ return;
88 } 106​ }
89 107​
90 qCDebug(KIOEXEC) << "Uploading" << path << "to" << dest; 108​ qCDebug(KIOEXEC) << "Uploading" << path << "to" << dest;
91 auto job = KIO::copy(QUrl::fromLocalFile(path), dest); 109​ auto job = KIO::copy(QUrl::fromLocalFile(path), dest);
92 connect(job, &KJob::result, this, [](KJob *job) { 110​ connect(job, &KJob::result, this, [](KJob *job) {
93 if (job->error()) { 111​ if (job->error()) {
94 KMessageBox::error(nullptr, job->errorString()); 112​ KMessageBox::error(nullptr, job->errorString());
95 } 113​ }
96 }); 114​ });
97} 115​}
98 116​
99void KIOExecd::slotDeleted(const QString &path) 117​void KIOExecd::removeWatchAndDirectory(const QString &tempFilePath, const QString &command)
100{ 118​{
101 if (!m_watched.contains(path)) { 119​ if (!m_watched.contains(tempFilePath)) {
102 return; 120​ return;
103 } 121​ }
104 122​
105 qCDebug(KIOEXEC) << "Going to forget" << path; 123​ m_appCounter.take(command);
106 m_watcher->removeFile(path); 124​ // If the command does not have more opened files, stop watching
107 m_watched.remove(path); 125​ if (!m_appCounter.contains(command)) {
126​ // Remove all the directories used by command
anthonyfieroniUnsubmitted
Done

Better to me, make a class variable single shot timer, then when you add in deleted start it if not, when it expires delete what you do, at last check if deleted is not empty just restart it. Make 30s predefined.

anthonyfieroni: Better to me, make a class variable single shot timer, then when you add in deleted start it if…
127​ for (auto it = m_openedBy.constBegin(); it != m_openedBy.constEnd(); ++it) {
128​ if (it.key() == command) {
129​ m_watcher->removeFile(it.value());
130​ m_watched.remove(it.value());
131​ QFileInfo info(it.value());
elvisangelaccioUnsubmitted
Done

Maybe constBegin()/constEnd() here?

elvisangelaccio: Maybe `constBegin()`/`constEnd()` here?
jtamateAuthorUnsubmitted
Not Done

I always thought const iterators were meant to be used when the whole container is considered ReadOnly, not only its elements, looks like the compiler agrees:
kioexecd.cpp:138:36: error: no matching function for call to ‘QMap<QString, QDateTime>::erase(QMap<QString, QDateTime>::const_iterator&)’

jtamate: I always thought const iterators were meant to be used when the whole container is considered…
132​ const auto parentDir = info.path();
133​ qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" << info.fileName();
134​ QDir(parentDir).removeRecursively();
dfaureUnsubmitted
Not Done

Move the call to currentDateTime() outside of the loop, so it happens only once.
It's much more costly than one might think (because of timezone conversion, which uses tzset() etc.)

dfaure: Move the call to currentDateTime() outside of the loop, so it happens only once. It's much more…
brunsUnsubmitted
Not Done

makes me wonder, wouldn't it be better to use currentDateTimeUtc() throughout? We are not interested in absolute time here anyway.

bruns: makes me wonder, wouldn't it be better to use currentDateTimeUtc() throughout? We are not…
135​ }
136​ }
137​ m_openedBy.remove(command);
138​ }
anthonyfieroniUnsubmitted
Not Done

Indentation

anthonyfieroni: Indentation
anthonyfieroniUnsubmitted
Done

Also for loop should looks like:

for (it = begin(); it != end();) {
    if () {
        it = erase(it);
    } else {
        ++it;
    }
}
anthonyfieroni: Also for loop should looks like: ``` for (it = begin(); it != end();) { if () { it…
brunsUnsubmitted
Not Done

Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) complexity during erase.
Or in this specific case, std::partition/{iterate over removed}/std::erase

bruns: Even better use the std::remove_if/std::erase pattern, which avoids O(n^2) complexity during…
108} 139​}
109 140​
110#include "kioexecd.moc" 141​#include "kioexecd.moc"
111 142​
anthonyfieroniUnsubmitted
Done

lvalue = rvalue (i mean space between =)

anthonyfieroni: lvalue = rvalue (i mean space between =)