KProcessInfoList -- add proclist backend for FreeBSD

Authored by tcberner on May 11 2019, 9:15 PM.



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 *********

Diff Detail

R244 KCoreAddons
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
tcberner created this revision.May 11 2019, 9:15 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 11 2019, 9:15 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
tcberner requested review of this revision.May 11 2019, 9:15 PM
tcberner added inline comments.May 11 2019, 9:16 PM

^ that should probably be 2-clause...

tcberner updated this revision to Diff 57915.May 11 2019, 9:19 PM

Remove commented line.

apol added a subscriber: apol.May 12 2019, 12:32 AM
apol added inline comments.
59 ↗(On Diff #57915)

doesn't that also need #if HAVE_PROCSTAT?

pino added a subscriber: pino.May 12 2019, 4:54 AM

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:
In addition to that, IMHO you can simplify all the various statements like

type foo;
foo = ...;


type foo = ...;
170–172 ↗(On Diff #57915)

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

tcberner updated this revision to Diff 57921.May 12 2019, 9:28 AM
  • Switch to 2-Clause
  • Move procstat backend into its own file
  • Remove now unnecessary config-kprocesslist.cmake
tcberner marked an inline comment as done.May 12 2019, 9:32 AM

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

adridg added inline comments.May 13 2019, 8:29 PM
56 ↗(On Diff #57921)

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.


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

pino added a comment.May 13 2019, 8:31 PM

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.

adridg added inline comments.Jun 23 2019, 10:01 PM

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
        pstat = procstat_open_sysctl();
    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.


Similarly, you could

struct ProcStatProcesses
    ProcStatProcesses(ProcStat& pstat) : parent(pstat)
        procs = procstat_getprocs(parent.pstat, KERN_PROC_PROC, 0, &proc_count);
        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.

tcberner updated this revision to Diff 68138.Oct 17 2019, 1:50 PM
  • procstat: add FindProcstat.cmake
  • procstat: add procstat backend
  • procstat: add procstat option
  • procstat: add linkage for procstat backend

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

adridg accepted this revision.Oct 17 2019, 2:21 PM

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)

This revision is now accepted and ready to land.Oct 17 2019, 2:21 PM
This revision was automatically updated to reflect the committed changes.