KProcessInfoList -- add proclist backend for FreeBSD
Needs ReviewPublic

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


Group Reviewers

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
No Linters Available
No Unit Test Coverage
Build Status
Buildable 11758
Build 11776: arc lint + arc unit
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.

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 = ...;

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

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.