D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

This revision was automatically updated to reflect the committed changes.
Closed by commit R244:b1d27cabc2a9: KProcessInfoList -- add proclist backend 
for FreeBSD (authored by tcberner).

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21146?vs=68138=68151

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindProcstat.cmake
  src/lib/CMakeLists.txt
  src/lib/util/kprocesslist_unix_procstat.cpp
  src/lib/util/kprocesslist_unix_procstat_p.h

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Adriaan de Groot]

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


  Let's get this in, and then quibble about how much commentary needs to be 
written for the `_p.h` file (since, looking back, it's a bit heavy on the C++ 
fancyness -- my Opinions have changed a little)

REPOSITORY
  R244 KCoreAddons

BRANCH
  procstat_v3

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner added a comment.


  @adridg I've addressed your comments [finally].

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner updated this revision to Diff 68138.
tcberner added a comment.


  - procstat: add FindProcstat.cmake
  - procstat: add procstat backend
  - procstat: add procstat option
  - procstat: add linkage for procstat backend

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21146?vs=57921=68138

BRANCH
  procstat_v3

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindProcstat.cmake
  src/lib/CMakeLists.txt
  src/lib/util/kprocesslist_unix_procstat.cpp
  src/lib/util/kprocesslist_unix_procstat_p.h

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Adriaan de Groot]

adridg added inline comments.

INLINE COMMENTS

> adridg wrote in kprocesslist_unix_procstat.cpp:38
> I absolutely have Opinions on C++ style that apply here, so I'm going to talk 
> to Tobias elsewhere.

In order to simplify cleanup, I'd use a RAII class as well:

  /** @brief Wrapper around procstat_open_sysctl()
   * 
   * Hangs on to a procstat instance for its friend classes, and closes
   * it on destruction. Cast to bool to check for success.
   */
  struct ProcStat
  {
  ProcStat()
  {
  pstat = procstat_open_sysctl();
  }
  ~ProcStat()
  {
  procstat_close(pstat);
  }
  operator bool() const
  {
  return pstat;
  }
  
  struct procstat *pstat;
  };

Lines 37-42 become `ProcStat p; if (!p) return rc;`, but more importantly you 
can drop cleanup from other return paths since the destructor handles it.

> kprocesslist_unix_procstat.cpp:46
> +struct kinfo_proc *procs;
> +procs = procstat_getprocs(pstat, KERN_PROC_PROC, 0, _count);
> +

Similarly, you could

  struct ProcStatProcesses
  {
  public:
  ProcStatProcesses(ProcStat& pstat) : parent(pstat)
  {
  procs = procstat_getprocs(parent.pstat, KERN_PROC_PROC, 0, 
_count);
  }
  ~ProcStatProcesses()
  {
  if (procs)
  {
  procstat_freeprocs(parent.pstat, procs);
  }
  }
  operator bool() const
  {
  return procs && proc_count > 0;
  }
  
  ProcStat& parent;
  unsigned int proc_count;
  struct kinfo_proc *procs;
  }

I'd probably also write an iterator for it, returning KProcessInfo from 
`operator*()`, so that the magic is hidden away in small classes and the 
overall algorithm becomes a very short bit of text. You might call that 
over-engineered, since there's nothing wrong with the code as-written.

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Pino Toscano]

pino added a comment.


  In D21146#463948 , @tcberner wrote:
  
  > Gargh, the code `kdelibs` code formatter changed a lot more in 
`kprocesslist_unix_proc.cpp` than wanted :/
  
  
  Then just rename kprocesslist_unix.cpp to kprocesslist_unix_proc.cpp, with no 
further changes.

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Adriaan de Groot]

adridg added inline comments.

INLINE COMMENTS

> kprocesslist_unix_proc.cpp:56
>  #else
> +#  ifdef Q_OS_FREEBSD
> +static const char formatC[] = "pid,state,user,command";

Are there situations where you would end up here? I mean, libprocstat is in 
base, since 9.0, so it is unlikely to be gone at any point.

> kprocesslist_unix_procstat.cpp:38
> +struct procstat *pstat;
> +pstat = procstat_open_sysctl();
> +

I absolutely have Opinions on C++ style that apply here, so I'm going to talk 
to Tobias elsewhere.

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

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


  Gargh, the code `kdelibs` code formatter changed a lot more in 
`kprocesslist_unix_proc.cpp` than wanted :/

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner updated this revision to Diff 57921.
tcberner added a comment.


  - Switch to 2-Clause
  - Move procstat backend into its own file
  - Remove now unnecessary config-kprocesslist.cmake

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21146?vs=57915=57921

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindProcstat.cmake
  src/lib/CMakeLists.txt
  src/lib/util/kprocesslist_unix.cpp
  src/lib/util/kprocesslist_unix_proc.cpp
  src/lib/util/kprocesslist_unix_procstat.cpp

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Pino Toscano]

pino added a comment.


  What about create a separate kprocesslist_libprocstat.cpp (or so) to 
implement `KProcessList::processInfoList()` using libprocstat, instead of 
overloading the existing kprocesslist_unix.cpp?
  
  Also, please adjust the coding style to the kdelibs coding style: 
https://community.kde.org/Policies/Kdelibs_Coding_Style
  In addition to that, IMHO you can simplify all the various statements like
  
type foo;
foo = ...;
  
  to
  
type foo = ...;

INLINE COMMENTS

> kprocesslist_unix.cpp:170-172
> +#ifdef HAVE_PROCSTAT
> +return unixProcessListKinfoProcStat();
> +#endif

this makes the rest of the function as unreachable code, and compilers might 
warn about that

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: pino, apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> kprocesslist_unix.cpp:59
>  
> +KProcessInfoList unixProcessListKinfoProcStat()
> +{

doesn't that also need `#if HAVE_PROCSTAT`?

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner updated this revision to Diff 57915.
tcberner added a comment.


  Remove commented line.

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21146?vs=57913=57915

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindProcstat.cmake
  src/lib/CMakeLists.txt
  src/lib/util/config-kprocesslist.h.cmake
  src/lib/util/kprocesslist_unix.cpp

To: tcberner, #freebsd, adridg, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner added inline comments.

INLINE COMMENTS

> FindProcstat.cmake:18
> +#documentation and/or other materials provided with the distribution.
> +# 3. Neither the name of the University nor the names of its contributors
> +#may be used to endorse or promote products derived from this software

^ that should probably be 2-clause...

REPOSITORY
  R244 KCoreAddons

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

To: tcberner, #freebsd, adridg, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D21146: KProcessInfoList -- add proclist backend for FreeBSD

[Tobias C. Berner]

tcberner created this revision.
tcberner added reviewers: FreeBSD, adridg, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
tcberner requested review of this revision.

REVISION SUMMARY
  This adds
  
  - unixProcessListKinfoProcStat() to KProcessInfoList to query process info 
via FreeBSD's procstat library
  - FindProcstat.cmake and config-kprocesslist.h.cmake to handle finding and 
using it
  
  - Additionally, the ps-argument in unixProcessListPS was changed to 
'commmand' form 'cmd' for FreeBSD.
  
  - Todo: make it nicer :)
  
  See D20007 

TEST PLAN
Totals: 6 passed, 0 failed, 0 skipped, 0 blacklisted, 198ms
* Finished testing of KProcessListTest *

REPOSITORY
  R244 KCoreAddons

BRANCH
  master

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

AFFECTED FILES
  CMakeLists.txt
  cmake/FindProcstat.cmake
  src/lib/CMakeLists.txt
  src/lib/util/config-kprocesslist.h.cmake
  src/lib/util/kprocesslist_unix.cpp

To: tcberner, #freebsd, adridg, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns