D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

This revision was automatically updated to reflect the committed changes.
jtamate marked an inline comment as done.
Closed by commit R241:820f622e86bb: kioexecd: watch for creations or 
modifications of the temporary files (authored by jtamate).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41066=41110

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate marked an inline comment as done.
jtamate added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kioexecd.cpp:129
> Maybe `constBegin()`/`constEnd()` here?

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::erase(QMap::const_iterator&)’

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kioexecd.cpp:129
> +// check if the deleted (and not recreated) files where deleted 30s ago 
> or more
> +for (auto it = m_deleted.begin(); it != m_deleted.end();) {
> +if (it.value().msecsTo(currentDateTime) >= predefinedTimeout) {

Maybe `constBegin()`/`constEnd()` here?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41066.
jtamate added a comment.


  Use QDateTime::currentDateTimeUtc() instead of QDateTime::currentDateTime().

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41035=41066

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> dfaure wrote in kioexecd.cpp:133
> 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.)

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added a comment.


  May I commit or should I wait for @elvisangelaccio to accept the changes? 
This time I have read the arc message, that is usually something about non 
tracked files:
  Revision 'D15180 : kioexecd: watch for 
creations or modifications of the temporary files' has not been accepted. 
Continue anyway? [y/N]
  
  > 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
  
  I assume QMap is like a std::map. I've seen in this cppreference page 
 that these algorithms 
cannot be used with associative containers such as std::set and std::map.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in kioexecd.cpp:134
> Also for loop should looks like:
> 
>   for (it = begin(); it != end();) {
>   if () {
>   it = erase(it);
>   } else {
>   ++it;
>   }
>   }

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: bruns, anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, 
ngraham

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Thanks!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41035.
jtamate added a comment.


  Tested, dirty is not signaled when created (at least I didn't saw the dialog 
for uploading the modified file).
  Removed the header.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41030=41035

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  No worries, that's what code review is for :-)
  
  Now this is starting to look good ;)

INLINE COMMENTS

> kioexecd.cpp:88
> +
> +slotDirty(path);
> +}

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 ;)

> kioexecd.h:28
>  #include 
> +#include 
> +#include 

remove

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41030.
jtamate added a comment.


  Why the mutex? I interpreted that QTimer work as an interrumpt (wrong) 
instead of generating events in the event queue (right).
  So I've gone through all the possible mistakes one can do here:
  
  - Design mistakes
  - Security mistakes
  - Misunderstanding the API.
  
  Definitely, I need more vacation.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=41026=41030

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kioexecd.cpp:36
>  
> +const int predefinedTimeout = 3; // 30s
> +

static const int...

> kioexecd.cpp:86
> +{
> +m_deleted_mutex.lock();
> +m_deleted.remove(path);

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

> kioexecd.cpp:133
> +for (auto it = m_deleted.begin(); it != m_deleted.end();) {
> +if (it.value().msecsTo(QDateTime::currentDateTime()) >= 
> predefinedTimeout) {
> +qCDebug(KIOEXEC) << "Going to forget" << it.key();

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.)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 41026.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  Do not delete recursively.
  Do not delete the file after 30s (we know it is already deleted).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40961=41026

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> jtamate wrote in kioexecd.cpp:65
> 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).

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in kioexecd.cpp:65
> 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`).

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).

> anthonyfieroni wrote in kioexecd.cpp:85-88
> Now it's not needed 'remove' will do the work

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kioexecd.cpp:65
>  qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" 
> << info.fileName();
> -QFile::remove(it.key());
> -QDir().rmdir(parentDir);
> +QDir(parentDir).removeRecursively();
>  }

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`).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40961.
jtamate added a comment.


  I missed that part, sorry.
  It is safer that way, the start(time) method was added in Qt 5.8.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40960=40961

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added a comment.


  m_timer.start(predefinedTimeout) should be m_timer.start() in 2 places, but 
let's see @elvisangelaccio and @dfaure comments.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40960.
jtamate marked 3 inline comments as done.
jtamate added a comment.


  Done.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40952=40960

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kioexecd.cpp:53
>  connect(m_watcher, ::deleted, this, ::slotDeleted);
> +m_timer.setSingleShot(true);
> +connect(_timer, ::timeout, this, 
> ::slotCheckDeletedFiles);

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

> kioexecd.cpp:85-88
> +if (!m_deleted.contains(path)) {
> +m_deleted_mutex.unlock();
> +return;
> +}

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

> kioexecd.cpp:143
> +QDir(parentDir).removeRecursively();
> +it=m_deleted.erase(it);
> +} else {

lvalue = rvalue (i mean space between =)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40952.
jtamate marked 4 inline comments as done.
jtamate added a comment.


  Implemented Anthony comments/suggestions.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40908=40952

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kioexecd.cpp:80
> +{
> +if (!m_deleted.contains(path)) {
> +return;

Contains should be also in the guard.

> kioexecd.h:40
>  virtual ~KIOExecd();
> +void clearDir(const QString );
>  

Unused?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kioexecd.cpp:134
> +QDir(parentDir).removeRecursively();
> +it=m_deleted.erase(it);
> +}

Also for loop should looks like:

  for (it = begin(); it != end();) {
  if () {
  it = erase(it);
  } else {
  ++it;
  }
  }

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> kioexecd.cpp:122
> +m_deleted_mutex.unlock();
> +QTimer::singleShot(31000, [this]() { // 31s
> +m_deleted_mutex.lock();

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40908.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  As I'm never sure if a timed execution can happen in the middle of other 
execution, I've added a mutex for m_deleted.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40888=40908

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added a comment.


  In D15180#319314 , @anthonyfieroni 
wrote:
  
  > I'm unhappy with that stop watching is on exit == 0, so when it's not, 
somehow, containers will continue to grow, it'll result in higher memory usage 
and slower performance. So stop watching should not depend on process return 
code, also same command should not stop container to shrink, that's my opinion, 
but i can miss something.
  
  
  I desist from this path, it has a big flaw: What if the tabbed application is 
already running? All the temporary files will be deleted immediately. :-(
  I'll try this other path: allowing the application a generous amount of time 
(30s) to recreate the deleted file, otherwise deleting the temporary directory. 
Unfortunately, the temporary directories will not shrink unless the app deletes 
the temporary file.
  I guess the only complete solution is to implement use fuse (where available) 
.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Anthony Fieroni]

anthonyfieroni added a comment.


  I'm unhappy with that stop watching is on exit == 0, so when it's not, 
somehow, containers will continue to grow, it'll result in higher memory usage 
and slower performance. So stop watching should not depend on process return 
code, also same command should not stop container to shrink, that's my opinion, 
but i can miss something.

INLINE COMMENTS

> kioexecd.cpp:138
> +m_openedBy.remove(command);
> + }
>  }

Indentation

> main.cpp:261
>  qDebug() << "about to delete" << parentDir << "containing" << 
> info.fileName();
> -QFile(QFile::encodeName(src)).remove();
> -QDir().rmdir(parentDir);
> + QDir(parentDir).removeRecursively();
>  }

Indentation

> main.cpp:263
>  }
> +if (mUseDaemon &&  exit_code == 0) {
> +OrgKdeKIOExecdInterface 
> kioexecd(QStringLiteral("org.kde.kioexecd"), 
> QStringLiteral("/modules/kioexecd"), QDBusConnection::sessionBus());

Extra space after &&

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, 
bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40888.
jtamate added a reviewer: elvisangelaccio.
jtamate added a comment.


  Updated with the code that handles m_openedBy right.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40779=40888

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio
Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Elvis Angelaccio]

elvisangelaccio added a comment.


  I don't understand what's the counter used for. Can you explain why we need 
it and can you update the Test plan accordingly?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate added inline comments.

INLINE COMMENTS

> broulik wrote in kioexecd.cpp:58
> 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

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~

> broulik wrote in kioexecd.cpp:73
> 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.

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

I've implemented an unwatch dbus call.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate updated this revision to Diff 40779.
jtamate marked 3 inline comments as done.
jtamate edited the summary of this revision.
jtamate edited the test plan for this revision.
jtamate added a comment.


  I'm not sure if I have to keep compatibility in the dbus calls, therefore the 
old one is still there.

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15180?vs=40737=40779

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h
  src/kioexec/main.cpp

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> jtamate wrote in kioexecd.cpp:58
> Why not?
> It should be just removing the temporary file copy and it's backups.

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

> jtamate wrote in kioexecd.cpp:73
> 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.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate marked 2 inline comments as done.
jtamate added inline comments.

INLINE COMMENTS

> broulik wrote in kioexecd.cpp:58
> I'm not sure that's a good idea

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

> broulik wrote in kioexecd.cpp:73
> You never unwatch the dir

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kioexecd.cpp:58
>  qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" 
> << info.fileName();
> -QFile::remove(it.key());
> -QDir().rmdir(parentDir);
> +QDir(parentDir).removeRecursively();
>  }

I'm not sure that's a good idea

> kioexecd.cpp:73
> +QDir qd(path);
> +m_watcher->addDir(qd.filePath(path), KDirWatch::WatchMode::WatchSubDirs);
>  m_watched.insert(path, QUrl(destUrl));

You never unwatch the dir

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

[Jaime Torres Amate]

jtamate created this revision.
jtamate added reviewers: Frameworks, broulik, ngraham, dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
jtamate requested review of this revision.

REVISION SUMMARY
  When a non KIO friendly program opens a non local file, the file is copied to 
an user temporary folder.
  Watch any creation or modification in that temporary folder. In that way, 
even with programs that delete the old file and create a new one when saving 
the file, the user is asked if he wants to upload the modified file.
  
  Delete recursively the temporary directories and any file they could have in 
the destructor.
  
  BUG: 397742

TEST PLAN
  Tested logging out and in several times and opening remote files with xed and 
libreoffice.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D15180

AFFECTED FILES
  src/kioexec/kioexecd.cpp
  src/kioexec/kioexecd.h

To: jtamate, #frameworks, broulik, ngraham, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns