Differential D15180 Diff 41110 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​
36​static const int predefinedTimeout = 30000; // 30s
dfaureUnsubmitted
Not Done

static const int...

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

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

anthonyfieroni: Also add interval here, setInterval, in other place just start()
54​ m_timer.setInterval(predefinedTimeout);
55​ connect(&m_timer, &QTimer::timeout, this, &KIOExecd::slotCheckDeletedFiles);
50} 56​}
51 57​
52KIOExecd::~KIOExecd() 58​KIOExecd::~KIOExecd()
53{ 59​{
60​ // Remove the remaining temporary files and if possible their parent directories
54 for (auto it = m_watched.constBegin(); it != m_watched.constEnd(); ++it) { 61​ for (auto it = m_watched.constBegin(); it != m_watched.constEnd(); ++it) {
55 QFileInfo info(it.key()); 62​ QFileInfo info(it.key());
56 const auto parentDir = info.path(); 63​ const auto parentDir = info.path();
57 qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" << info.fileName(); 64​ qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" << info.fileName();
58 QFile::remove(it.key()); 65​ QFile::remove(it.key());
59 QDir().rmdir(parentDir); 66​ QDir().rmdir(parentDir);
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
60 } 67​ }
61} 68​}
62 69​
63void KIOExecd::watch(const QString &path, const QString &destUrl) 70​void KIOExecd::watch(const QString &path, const QString &destUrl)
64{ 71​{
65 if (m_watched.contains(path)) { 72​ if (m_watched.contains(path)) {
66 qCDebug(KIOEXEC) << "Already watching" << path; 73​ qCDebug(KIOEXEC) << "Already watching" << path;
67 return; 74​ return;
68 } 75​ }
69 76​
70 qCDebug(KIOEXEC) << "Going to watch" << path << "for changes, remote destination is" << destUrl; 77​ qCDebug(KIOEXEC) << "Going to watch" << path << "for changes, remote destination is" << destUrl;
71 78​
79​ // Watch the temporary file for modifications, creations or deletions
72 m_watcher->addFile(path); 80​ m_watcher->addFile(path);
73 m_watched.insert(path, QUrl(destUrl)); 81​ m_watched.insert(path, QUrl(destUrl));
74} 82​}
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 83​
84​void KIOExecd::slotCreated(const QString &path)
85​{
86​ m_deleted.remove(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.
87​
88​ // When the file is recreated, it is not signaled as dirty.
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…
89​ slotDirty(path);
90​}
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…
91​
76void KIOExecd::slotDirty(const QString &path) 92​void KIOExecd::slotDirty(const QString &path)
77{ 93​{
78 if (!m_watched.contains(path)) { 94​ if (!m_watched.contains(path)) {
79 return; 95​ return;
80 } 96​ }
81 97​
82 const auto dest = m_watched.value(path); 98​ const auto dest = m_watched.value(path);
83 99​
Show All 13 Lines
97} 113​}
98 114​
99void KIOExecd::slotDeleted(const QString &path) 115​void KIOExecd::slotDeleted(const QString &path)
100{ 116​{
101 if (!m_watched.contains(path)) { 117​ if (!m_watched.contains(path)) {
102 return; 118​ return;
103 } 119​ }
104 120​
105 qCDebug(KIOEXEC) << "Going to forget" << path; 121​ m_deleted.insert(path, QDateTime::currentDateTimeUtc());
106 m_watcher->removeFile(path); 122​ m_timer.start();
107 m_watched.remove(path); 123​}
124​
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…
125​void KIOExecd::slotCheckDeletedFiles()
126​{
127​ const QDateTime currentDateTime = QDateTime::currentDateTimeUtc();
128​ // check if the deleted (and not recreated) files where deleted 30s ago or more
129​ for (auto it = m_deleted.begin(); it != m_deleted.end();) {
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…
130​ if (it.value().msecsTo(currentDateTime) >= predefinedTimeout) {
131​ qCDebug(KIOEXEC) << "Going to forget" << it.key();
132​ m_watcher->removeFile(it.key());
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…
133​ m_watched.remove(it.key());
134​ QFileInfo info(it.key());
135​ const auto parentDir = info.path();
136​ qCDebug(KIOEXEC) << "About to delete" << parentDir;
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…
137​ QDir().rmdir(parentDir);
138​ it = m_deleted.erase(it);
139​ } else {
140​ ++it;
anthonyfieroniUnsubmitted
Done

lvalue = rvalue (i mean space between =)

anthonyfieroni: lvalue = rvalue (i mean space between =)
141​ }
142​ }
143​ if (!m_deleted.isEmpty()) {
144​ m_timer.start();
145​ }
108} 146​}
109 147​
110#include "kioexecd.moc" 148​#include "kioexecd.moc"
111 149​