D21760: Add KListOpenFilesJob

[David Faure]

dfaure added a comment.


  The test fails on FreeBSD, in CI.
  
  
https://build.kde.org/job/Frameworks/view/Platform%20-%20FreeBSDQt5.13/job/kcoreaddons/job/kf5-qt5%20FreeBSDQt5.13/118/testReport/junit/projectroot/autotests/klistopenfilesjobtest_unix/
  
  Any plans for fixing it, or should I mark it as expected failure?

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, 
ngraham, bruns

D21760: Add KListOpenFilesJob

[Méven Car]

meven added a comment.


  Now we can use it :
  
  > dolphin D19989 
  > plasma-vault
  >  libksysguard (KLsofWidget)
  >  dataengines/devicenotifications/ksolidnotify

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas closed this revision.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas updated this revision to Diff 65856.
hallas added a comment.


  Updated @since

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65500=65856

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Faure]

dfaure added a comment.


  The tagging has been done, you can push.

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas added a comment.


  In D21760#526652 , @dfaure wrote:
  
  > This is good to go in :-)
  >
  > If you push it today it'll indeed be in 5.62, to be tagged tomorrow, 
otherwise we'll need to adjust the @since tag ;-)
  
  
  I think i prefer waiting to after the release so it has a chance to get some 
mileage before release :) I will make sure to update the @since tag.

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  This is good to go in :-)
  
  If you push it today it'll indeed be in 5.62, to be tagged tomorrow, 
otherwise we'll need to adjust the @since tag ;-)

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfilesjobtest_unix.cpp:34
> (minor) we never check that `new` succeeded, in Qt code.
> 
> The reasoning is that on desktop systems, with swap enabled, before the swap 
> is exhausted, the user will have given up and rebooted the machine anyway. I 
> know I do, many times a month :). So in practice, `new` can be considered to 
> always succeed.

Makes sense, this was a leftover from when the job was returned by a function 
instead of new-ing it directly :) Another reason for not checking is that new 
doesn't return nullptr on failure, instead it throws std::bad_alloc, to get 
nullptr on failure you need to use the nothrow version of new :)

> dfaure wrote in klistopenfilesjob.h:39
> [is it useful to repeat that sentence? I admit I'm no expert on Doxygen's 
> @brief command]

I don't think it is useful, so I have remove it

> dfaure wrote in klistopenfilesjob_win.cpp:26
> Won't be needed anymore once the getter returns by value :-)

Remove :)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas updated this revision to Diff 65500.
hallas marked 7 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65494=65500

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Faure]

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


  Yep, much simpler indeed.

INLINE COMMENTS

> klistopenfilesjobtest_unix.cpp:34
> +auto job = new KListOpenFilesJob(path.path());
> +QVERIFY(job);
> +job->exec();

(minor) we never check that `new` succeeded, in Qt code.

The reasoning is that on desktop systems, with swap enabled, before the swap is 
exhausted, the user will have given up and rebooted the machine anyway. I know 
I do, many times a month :). So in practice, `new` can be considered to always 
succeed.

> klistopenfilesjobtest_unix.cpp:80
> +{
> +setenv(name_.latin1(), newValue.data(), 1);
> +}

You can use `qputenv`, more portable (in case we ever enable this code on 
Windows), safer in terms of life cycle of the strings (I mean in general, I 
know it isn't a problem here).

> klistopenfilesjob.h:37
> +/**
> + * @brief Provides information about processes that has open files in a 
> given path or subdirectory of path.
> + *

s/has/have/

> klistopenfilesjob.h:39
> + *
> + * Provides information about processes that has open files in a given path 
> or subdirectory of path.
> + *

[is it useful to repeat that sentence? I admit I'm no expert on Doxygen's 
@brief command]

> klistopenfilesjob.h:41
> + *
> + * When start() is invoked it starts to collect information about processes 
> that has any files open in path or a
> + * subdirectory of path. When it is done the KJob::result signal is emitted 
> and the result can be retrieved with the

has -> have  (plural)

> klistopenfilesjob.h:57
> +void start() override;
> +const KProcessList::KProcessInfoList& processInfoList() const;
> +

Return by value rather than by const ref. Otherwise it makes it hard to one day 
write something like

  if (someCondition)
  return KProcessInfoList();
  return m_processInfoList;

The cost of increasing/decreasing a refcount is negligible, especially in code 
that starts a secondary process :-)

+ don't forget to document the method

> klistopenfilesjob_unix.cpp:93
> +const QDir path;
> +bool hasEmittedResult_;
> +QProcess lsofProcess;

Inconsistent: two member variables have a trailing '_', the others don't.

In KF5 code it's usual to either have no prefix, or have `m_` as prefix (but 
never a suffix).

> klistopenfilesjob_win.cpp:26
> +public:
> +KProcessList::KProcessInfoList processInfoList;
> +};

Won't be needed anymore once the getter returns by value :-)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas updated this revision to Diff 65494.
hallas added a comment.


  Review comments, renamed the files to match the class name

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65330=65494

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilesjobtest_unix.cpp
  autotests/klistopenfilesjobtest_unix.h
  autotests/klistopenfilesjobtest_win.cpp
  autotests/klistopenfilesjobtest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfilesjob.h
  src/lib/util/klistopenfilesjob_unix.cpp
  src/lib/util/klistopenfilesjob_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas marked an inline comment as done.
hallas added a comment.


  In D21760#525842 , @dfaure wrote:
  
  > Yes, the filenames should match the classname, obviously :)
  >
  > I did a deeper review of the KJob usage and I have two more comments, sorry 
for not taking the time to do this earlier...
  
  
  No problem ;) I would rather have this change go in slow and correct than 
quick and dirty :)

INLINE COMMENTS

> dfaure wrote in klistopenfiles_unix.cpp:57
> Emitting `result` twice would be a very bad idea, a KJob can only emit 
> `result` once.
> 
> But QProcess can emit `errorOccurred` followed by `finished` (e.g. if the 
> subprocess crashes, from what I can see in qprocess.cpp).
> I think we want to only qWarning() in this method, and let `lsofFinished` 
> take care of finishing the job.

Good catch! Actually it turns out to be a tad more complicated ;) QProcess will 
only emit finished if the process actually started, so if you have the case 
where lsof is not installed you will only get the errorOccurred signal and not 
finished, but if it crashes you get both. I actually added a unit test to test 
the first case (where lsof is not found), and I ended up making a solution 
where we simply remember if we have emitted the result and only emit it if we 
haven't done so before.

> dfaure wrote in klistopenfiles_unix.cpp:73
> The class would be slightly easier to use if this was a getter instead of a 
> signal.
> 
> Because then, in a unittest, you can just do `job->exec()` and then 
> `job->processInfoList()`, no spy needed.
> In (GUI) application code you still need to connect to a signal, but then the 
> usual KJob::result signal is enough, instead of two signals 
> (`processInfoAvailable` in case of success, and `result` to handle failure).
> 
> See `KIO::StatJob::statResult()` for an example.
> 
> Signals emitted by jobs are useful if they happen during the job lifetime 
> (e.g. progress signals). If they happen at the same time as `result`, then 
> it's easier to use `result` as the notification signal.
> 
> The downside is one more member variable, but it'll be very short-lived so I 
> wouldn't worry about memory usage.

Agree, I have refactored the code to do this and it is **much** simpler :)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Faure]

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


  Yes, the filenames should match the classname, obviously :)
  
  I did a deeper review of the KJob usage and I have two more comments, sorry 
for not taking the time to do this earlier...

INLINE COMMENTS

> klistopenfiles_unix.cpp:57
> +job->setErrorText(QObject::tr("Failed to execute `lsof' error code 
> %1").arg(processError));
> +job->emitResult();
> +}

Emitting `result` twice would be a very bad idea, a KJob can only emit `result` 
once.

But QProcess can emit `errorOccurred` followed by `finished` (e.g. if the 
subprocess crashes, from what I can see in qprocess.cpp).
I think we want to only qWarning() in this method, and let `lsofFinished` take 
care of finishing the job.

> klistopenfiles_unix.cpp:62
> +QStringList blockApps;
> +QString out(QString::fromLocal8Bit(lsofProcess.readAll()));
> +QStringList pidList = out.split(QRegExp(QStringLiteral("\\s+")), 
> QString::SkipEmptyParts);

const

> klistopenfiles_unix.cpp:73
> +}
> +emit job->processInfoAvailable(path.path(), processInfoList);
> +job->emitResult();

The class would be slightly easier to use if this was a getter instead of a 
signal.

Because then, in a unittest, you can just do `job->exec()` and then 
`job->processInfoList()`, no spy needed.
In (GUI) application code you still need to connect to a signal, but then the 
usual KJob::result signal is enough, instead of two signals 
(`processInfoAvailable` in case of success, and `result` to handle failure).

See `KIO::StatJob::statResult()` for an example.

Signals emitted by jobs are useful if they happen during the job lifetime (e.g. 
progress signals). If they happen at the same time as `result`, then it's 
easier to use `result` as the notification signal.

The downside is one more member variable, but it'll be very short-lived so I 
wouldn't worry about memory usage.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> klistopenfiles.h:23
> +
> +#ifndef KLISTOPENFILES_H
> +#define KLISTOPENFILES_H

Should these files be renamed to klistopenfilesjob (along with tests etc.) now 
that is what the class is called?

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas added a comment.


  In D21760#524860 , @dfaure wrote:
  
  > Given that the namespace doesn't contain anything else anymore, I would 
just get rid of it, and provide a single class, KListOpenFilesJob.
  
  
  Done :)

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas updated this revision to Diff 65330.
hallas added a comment.


  Removed KListOpenFiles namespace and renamed ListOpenFilesJob to 
KListOpenFilesJob

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=65284=65330

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest_unix.cpp
  autotests/klistopenfilestest_unix.h
  autotests/klistopenfilestest_win.cpp
  autotests/klistopenfilestest_win.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.h
  src/lib/util/klistopenfiles_unix.cpp
  src/lib/util/klistopenfiles_win.cpp

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D21760: Add KListOpenFilesJob

[David Hallas]

hallas retitled this revision from "Add KListOpenFiles::ListOpenFilesJob" to 
"Add KListOpenFilesJob".
hallas edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns