D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfilestest_unix.cpp:83
> I usually just use "/does/not/exist" as a path ;-)
> 
> This might actually be better because Windows has weird race conditions with 
> the filesystem stuff.

This test is unix only anyway so there shouldn't be any race conditions. But it 
might be a little more expressive to use "/does/not/exist" so I have changed it 
anyway ;)

> dfaure wrote in klistopenfiles.h:94
> KIO is designed like this for some reason, but I would just document the job 
> constructor and let people create the job with new, outside KIO.
> 
> That's e.g. what akonadi does.

Would it make sense to rename the ListOpenFilesJob class to simply Job? It is 
already inside a KListOpenFiles namespace so the current name seems a little 
long and duplicated? Do we have any general naming convention for these things?

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas updated this revision to Diff 65284.
hallas marked 3 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

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

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 KListOpenFiles::listProcessesWithOpenFiles

[David Faure]

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


  Nice job. Final comments.

INLINE COMMENTS

> klistopenfilestest_unix.cpp:51
> +QCOMPARE(processInfoAvailableSpy.at(0).at(0).value(), 
> path.path());
> +KProcessList::KProcessInfoList processInfoList = 
> processInfoAvailableSpy.at(0).at(1).value();
> +QVERIFY(!processInfoList.empty());

prepend `const` so begin/end below don't detach

> klistopenfilestest_unix.cpp:83
> +QTemporaryDir tempDir;
> +tempDir.remove();
> +auto job = KListOpenFiles::listProcessesWithOpenFiles(tempDir.path());

I usually just use "/does/not/exist" as a path ;-)

This might actually be better because Windows has weird race conditions with 
the filesystem stuff.

> klistopenfiles.h:94
> + */
> +KCOREADDONS_EXPORT ListOpenFilesJob *listProcessesWithOpenFiles(const 
> QString );
> +

KIO is designed like this for some reason, but I would just document the job 
constructor and let people create the job with new, outside KIO.

That's e.g. what akonadi does.

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  I have added a minimal Windows implementation which always emits an error, 
along with a unit test. Please review it thoroughly and then I think it is 
ready to land :)

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas updated this revision to Diff 65237.
hallas marked 2 inline comments as done.
hallas added a comment.


  Review comments. Added minimal Windows implementation which basically always 
reports failure with the error code Unsupported.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=64973=65237

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 KListOpenFiles::listProcessesWithOpenFiles

[David Faure]

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


  The KJob usage turned out exactly like I thought it would, I still think it's 
the right thing to do :-)

INLINE COMMENTS

> hallas wrote in klistopenfiles.cpp:29
> The primary motivation was memory-management, but I could also just make the 
> d-pointer in ListOpenFilesJob a QScopedPointer, would that be better?

Yes, that would be more lightweight.

> hallas wrote in klistopenfiles.cpp:44
> I have added the error enum entries, but I am a little hesitant to add to 
> error texts since KCoreAddons doesn't today depend on KDE::I18n and this 
> would introduce that dependency, do we really want that? When I look at the 
> KIO classes then they usually set som non-translated string as the error 
> text, would that be an option?

My bad. In KCoreAddons should should use tr()+arg() rather than i18n.

KIO doesn't have non-translated strings. It reimplements errorString() to 
combine the errorText (usually a path or URL) with a translated string based on 
the error code.
But the base KJob doesn't have any of that, it just defaults to errorText, so 
you *need* to use tr().

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  One thing, when this is ready to land I will address the Windows support so 
that we do not get broken builds :)

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas updated this revision to Diff 64973.
hallas marked 2 inline comments as done.
hallas added a comment.


  Review comments

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=64911=64973

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  @dfaure  - Overall, what do you think about the approach of subclassing KJob? 
Did it turn out like you had thought? And is this the solution we should go 
with, or was one of the other solutions better?

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:29
> I wonder if this actually needs to be a QObject, given that you use 
> connect-to-pointer-to-member-function?

The primary motivation was memory-management, but I could also just make the 
d-pointer in ListOpenFilesJob a QScopedPointer, would that be better?

> dfaure wrote in klistopenfiles.cpp:44
> KIO::ERR_DOES_NOT_EXIST would fit well here, but that's in kio...
> 
> I suggest adding enum { ERR_DOES_NOT_EXIST = KJob::UserDefinedError + 11 }
> in the header file for this job and using that here.
> 
> And yes, you should do something like
> 
>   setErrorText(i18n("Directory does not exist: %1", path));

I have added the error enum entries, but I am a little hesitant to add to error 
texts since KCoreAddons doesn't today depend on KDE::I18n and this would 
introduce that dependency, do we really want that? When I look at the KIO 
classes then they usually set som non-translated string as the error text, 
would that be an option?

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 KListOpenFiles::listProcessesWithOpenFiles

[Méven Car]

meven 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 KListOpenFiles::listProcessesWithOpenFiles

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> klistopenfiles.cpp:29
> +
> +class ListOpenFilesJobPrivate : public QObject
> +{

I wonder if this actually needs to be a QObject, given that you use 
connect-to-pointer-to-member-function?

> hallas wrote in klistopenfiles.cpp:44
> Should I also set an error string here? And what about defining custom error 
> codes, is that how we usually do?

KIO::ERR_DOES_NOT_EXIST would fit well here, but that's in kio...

I suggest adding enum { ERR_DOES_NOT_EXIST = KJob::UserDefinedError + 11 }
in the header file for this job and using that here.

And yes, you should do something like

  setErrorText(i18n("Directory does not exist: %1", path));

> klistopenfiles.cpp:53
> +{
> +job->setError(KJob::UserDefinedError);
> +job->emitResult();

+setErrorText()

> klistopenfiles.h:41
> +public:
> +explicit ListOpenFilesJob(QDir path);
> +~ListOpenFilesJob() override;

const QDir &  --- or actually, we use const QString & everywhere else for paths.

> klistopenfiles.h:51
> + * @param processInfoList The list of processes with open files in path
> + * @since 5.62
> + */

The @since should go into the (missing) class documentation, given that the 
whole class is new.

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> klistopenfiles.cpp:44
> +if (!path.exists()) {
> +job->setError(KJob::UserDefinedError);
> +job->emitResult();

Should I also set an error string here? And what about defining custom error 
codes, is that how we usually do?

> klistopenfiles.h:37
> +
> +class KCOREADDONS_EXPORT ListOpenFilesJob : public KJob
> +{

I am a little unsure if there is any other KJob functions that should be 
overwritten in this class? The current implementation is quite minimal, but 
maybe that is fine?

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  @meven , so I finally managed to rewrite this patch to use KJob instead. 
Please take a look at it again and see if this is better approach :)

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 KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

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


  Rewrote the code to use KJob

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21760?vs=59636=64911

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> meven wrote in klistopenfiles.cpp:39
> The qAsConst wrapping is missing it seems.

Yeah, I have only done this change locally ;)

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, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:39
> Not possible, due to the removeDuplicates() line.

The qAsConst wrapping is missing it seems.

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, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> klistopenfiles.cpp:26
> +
> +void listProcessesWithOpenFiles(QDir path, Callback callback)
> +{

Can you allow passing an optional context object, so when the caller gets 
deleted, you don't crash when the reply comes in?

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas marked 3 inline comments as done.
hallas added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:49
> This reminds me of KCoreAddons' new KProcessInfo class, although that one 
> doesn't actually do this.
> 
> I see that FreeBSD and OSX have lsof (and the man page doesn't say those 
> options are missing there) so it should be fine, everywhere except Windows.
> 
> In general I don't really like starting executables like this, but indeed 
> writing a clone of lsof sounds like much work.

I don't like starting executables either, it is slow, brittle and an implicit 
dependency - but I also think in this case it is the only feasible option :/

But is it ok not to support Windows? Or maybe the Windows implementation would 
emit a debug warning and then always emit an empty list?

> dfaure wrote in klistopenfiles.h:40
> Reading the unittest, I definitely agree that a signal would be better. Then 
> you could use QSignalSpy::wait() without the need to use QEventLoop by hand 
> :-)
> 
> This is really a job. I would inherit KJob for this, actually. It's the way 
> we do such things everywhere else.

Thanks for the feedback! I will look into making a solution using a signal 
instead and push an updated commit.

Regarding implementing this as a KJob subclass, do you have a good example of 
this? Would the result be delivered in a special signal, or is there a standard 
mechanism for this in KJob?

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a subscriber: cfeck.
hallas added a comment.


  In D21760#493817 , @cfeck wrote:
  
  > You wrote "ported from Device Notifier". I haven't check in detail yet, but 
do other copyright holders need to be mentioned?
  
  
  I have added the copyright holders from:
  plasma-workspace/dataengines/devicenotifications/ksolidnotify.cpp

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> broulik wrote in klistopenfiles.cpp:39
> Or just make the `pidList` `const` which it should be anyway

Not possible, due to the removeDuplicates() line.

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> dfaure wrote in klistopenfiles.cpp:39
> qAsConst(pidList) to avoid a detach

Or just make the `pidList` `const` which it should be anyway

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Faure]

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

INLINE COMMENTS

> klistopenfilestest.cpp:45
> +eventLoop.exec();
> +QVERIFY(processInfoList.empty() == false);
> +auto testProcessIterator = std::find_if(processInfoList.begin(), 
> processInfoList.end(), [](const KProcessList::KProcessInfo& info)

QVERIFY(!processInfoList.empty())

> klistopenfilestest.cpp:52
> +const auto& processInfo = *testProcessIterator;
> +QVERIFY(processInfo.isValid() == true);
> +QCOMPARE(processInfo.pid(), QCoreApplication::applicationPid());

The `== true` can be removed

(repeats)

> klistopenfiles.cpp:39
> +KProcessList::KProcessInfoList processInfoList;
> +for (const auto& pidStr : pidList) {
> +qint64 pid = pidStr.toLongLong();

qAsConst(pidList) to avoid a detach

> hallas wrote in klistopenfiles.cpp:49
> What should we do for Windows? Do we support any other platforms that doesn't 
> have lsof?

This reminds me of KCoreAddons' new KProcessInfo class, although that one 
doesn't actually do this.

I see that FreeBSD and OSX have lsof (and the man page doesn't say those 
options are missing there) so it should be fine, everywhere except Windows.

In general I don't really like starting executables like this, but indeed 
writing a clone of lsof sounds like much work.

> hallas wrote in klistopenfiles.h:40
> I am very much in doubt about what a nice interface for this should be? I was 
> thinking of an alternative approach where you would instantiate a class, 
> connect to a signal on the class and then invoke a method to start the 
> listing process, you would then receive the result on a slot instead. What do 
> you guys think?

Reading the unittest, I definitely agree that a signal would be better. Then 
you could use QSignalSpy::wait() without the need to use QEventLoop by hand :-)

This is really a job. I would inherit KJob for this, actually. It's the way we 
do such things everywhere else.

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Nathaniel Graham]

ngraham added reviewers: bruns, Plasma.

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Christoph Feck]

cfeck resigned from this revision.
cfeck added a comment.


  You wrote "ported from Device Notifier". I haven't check in detail yet, but 
do other copyright holders need to be mentioned?

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[Nathaniel Graham]

ngraham added reviewers: Frameworks, cfeck, dfaure.

REPOSITORY
  R244 KCoreAddons

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

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

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  Ping :D

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas added a comment.


  First of, this is WIP, I just wanted to share this early to get some 
feedback. The reasoning for this is to generalize functionality for running 
`lsof`, this is currently in use by the Device Notifier applet and I would like 
to use it in Dolphin to fix bug #189302. Also see the discussion in D19989 
 for details.

INLINE COMMENTS

> klistopenfilestest.cpp:31
> +
> +void KListOpenFilesTest::testOpenFiles()
> +{

Most of these tests wont work on Windows, so I need to fix that

> klistopenfiles.cpp:49
> +});
> +p->start(QStringLiteral("lsof"), {QStringLiteral("-t"), 
> QStringLiteral("+d"), path.path()});
> +}

What should we do for Windows? Do we support any other platforms that doesn't 
have lsof?

> klistopenfiles.h:40
> + */
> +KCOREADDONS_EXPORT void listProcessesWithOpenFiles(QDir path, Callback 
> callback);
> +

I am very much in doubt about what a nice interface for this should be? I was 
thinking of an alternative approach where you would instantiate a class, 
connect to a signal on the class and then invoke a method to start the listing 
process, you would then receive the result on a slot instead. What do you guys 
think?

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21760: Add KListOpenFiles::listProcessesWithOpenFiles

[David Hallas]

hallas created this revision.
hallas added reviewers: davidedmundson, broulik.
hallas added a project: Frameworks.
hallas requested review of this revision.

REVISION SUMMARY
  Add KListOpenFiles::listProcessesWithOpenFiles for retrieving the list
  of processes that has files open in the given path or a subdirectory of
  path. This code is ported from Device Notifier

TEST PLAN
  Unit Tests

REPOSITORY
  R244 KCoreAddons

BRANCH
  add_list_processes_with_open_files (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/klistopenfilestest.cpp
  autotests/klistopenfilestest.h
  src/lib/CMakeLists.txt
  src/lib/util/klistopenfiles.cpp
  src/lib/util/klistopenfiles.h

To: hallas, davidedmundson, broulik
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns