As we're using buffered I/O when writing kernel parameters, write errors
may get delayed until we close the `FILE` stream. As we are currently
outputting the key that is to be set disregarding the return value of
`close_stream`, we may end up in a situation where we report error and
success:
$ sysctl kernel.printk_ratelimit=100000000000000
sysctl: setting key "kernel.printk_ratelimit": error code 22
kernel.printk_ratelimit = 100000000000000
Fix the issue by only outputting the updated value in case
`close_stream` does not report an error.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
When writing to procfs via `proc_write` fails, we try to chunk the
buffer into smaller pieces to work around that issue. When searching for
the next location to split the buffer, though, we can underflow the
buffer in case the current offset is smaller than `LINELEN`. Fix the
issue by passing `cookie->offset` instead of `LINELEN` into `memrchr` in
case `cookie->offset` is smaller than `LINELEN`.
This bug can be triggered on musl-based systems, e.g. by executing
$ sysctl kernel.printk_ratelimit=1000000000000000
As the value is out-of-range, `write` will return an error and set
`errno` to `EINVAL`. As we're only trying to write a smallish buffer
with a length smaller than `LINELEN` and as the buffer does not contain
any newlines, the call
token = (char*)memrchr(cookie->buf+offset, '\n', LINELEN);
will underflow the buffer and crash the program.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
The `fprocopen` function allows users to specify a delimiter chacter
that is used to split very large input lines into smaller chunks. While
the code checks that the caller did actually supply the delimiter, it is
in fact never used to split the string. Instead, the hardcoded default
character ',' is always used to split the string.
Fix the issue by using `cookie->delim` instead.
As we're using buffered I/O when writing kernel parameters, write errors
may get delayed until we close the `FILE` stream. As we are currently
outputting the key that is to be set disregarding the return value of
`close_stream`, we may end up in a situation where we report error and
success:
$ sysctl kernel.printk_ratelimit=100000000000000
sysctl: setting key "kernel.printk_ratelimit": error code 22
kernel.printk_ratelimit = 100000000000000
Fix the issue by only outputting the updated value in case
`close_stream` does not report an error.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
When writing to procfs via `proc_write` fails, we try to chunk the
buffer into smaller pieces to work around that issue. When searching for
the next location to split the buffer, though, we can underflow the
buffer in case the current offset is smaller than `LINELEN`. Fix the
issue by passing `cookie->offset` instead of `LINELEN` into `memrchr` in
case `cookie->offset` is smaller than `LINELEN`.
This bug can be triggered on musl-based systems, e.g. by executing
$ sysctl kernel.printk_ratelimit=1000000000000000
As the value is out-of-range, `write` will return an error and set
`errno` to `EINVAL`. As we're only trying to write a smallish buffer
with a length smaller than `LINELEN` and as the buffer does not contain
any newlines, the call
token = (char*)memrchr(cookie->buf+offset, '\n', LINELEN);
will underflow the buffer and crash the program.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
The `fprocopen` function allows users to specify a delimiter chacter
that is used to split very large input lines into smaller chunks. While
the code checks that the caller did actually supply the delimiter, it is
in fact never used to split the string. Instead, the hardcoded default
character ',' is always used to split the string.
Fix the issue by using `cookie->delim` instead.
This patch just polishes the 'origin' comments for the
<STAT> header file. In particular those derived/unique
items (the 'SUM' guys) will now be properly explained.
[ in order to employ the 'derived from above' phrase ]
[ with their DELTA versions, all SUM items had to be ]
[ relocated (and some renamed). in turn, that had an ]
[ impact on many portions of the .c source file too. ]
Reference(s):
. summary calculations introduced
commit 2c86c4984a
. origins added to header files
commit 96d59cbf46
Signed-off-by: Jim Warner <james.warner@comcast.net>
This small change is an outgrowth of the research into
the bug represented by that merge request shown below.
With the master branch, a real buglet was subsequently
addressed. In this newlib branch, no bug existed since
the <stat> API relies solely on just cpus reflected in
(and parsed from) the kernel's /proc/stat pseudo file.
[ since that procps_stat_new() priming read about to ]
[ be performed will value info->cpus.total, there is ]
[ no need to separately invoke a procps_cpu_count(). ]
Reference(s):
https://gitlab.com/procps-ng/procps/merge_requests/82
Signed-off-by: Jim Warner <james.warner@comcast.net>
The merge request shown below prompted (thankfully) an
examination of our lxc containers logic in readproc.c.
As it turns out, the lxc folks changed that eyecatcher
used to identify containers within a task cgroup file.
So this patch, with little extra cost, will enable the
libprocps lxc_containers() guy to handle both strings.
[ additionally, I was shocked to find lxc allows the ]
[ eyecatcher to be changed at ./configure time. such ]
[ a provision has always existed. unfortunately, the ]
[ changed value was only available to root, assuming ]
[ one wished to tackle that undocumented liblxc api. ]
Reference(s):
. what prompted lxc support reevaluation
https://gitlab.com/procps-ng/procps/merge_requests/82
. original lxc support introduced
commit 0557504f9c
Signed-off-by: Jim Warner <james.warner@comcast.net>
I thank Guido Jäkel for raising the issue cited in the
merge request referenced below. While restoring 1 line
of code would produce the desired results, it does not
address the root cause of that problem he experienced.
The variable 'smp_num_cpus' was set by libprocps via a
sysconf(_SC_NPROCESSORS_ONLN) call. It was supposed to
represent total number of processors currently online.
It also served as the position in the Cpu_tics[] array
where the /proc/stat line #1 (cpu summary) was stored.
The variable 'Cpu_faux_tot' was valued by top based on
total individual cpus parsed from the /proc/stat file.
It serves as a fence post for Cpu_tics[] array access.
The problem Guido experienced results from a disparity
between those 2 variables, plus one instance where the
wrong variable was used in the summary_show() routine.
. Here is the real culprit, the actual incorrect code:
. summary_hlp(&Cpu_tics[Cpu_faux_tot], N_txt(WORD_a...
Which always should have been represented in this way:
. summary_hlp(&Cpu_tics[smp_num_cpus], N_txt(WORD_a...
------------------------------------------------------
The above 'disparity' might arise in any system when a
cpu is taken offline since there's a 3 second delay in
cpu and memory refreshes in an effort to reduce costs.
Usually this particular condition will be short lived.
However, there is a more persistent problem under lxc.
If a host cpu is taken offline and then brought online
again, within the container sysconf returns the proper
number of online processors. But, /proc/stat does not!
Sadly, I've yet to find a way to coax a container into
refreshing its /proc/stat, short of reboting the host.
[ might that represent a potential bug in lxc logic? ]
Reference(s):
https://gitlab.com/procps-ng/procps/merge_requests/82
Signed-off-by: Jim Warner <james.warner@comcast.net>
With-thanks-to: Guido Jäkel <G.Jaekel@DNB.DE>
The merge request shown below prompted (thankfully) an
examination of our lxc containers logic in readproc.c.
As it turns out, the lxc folks changed that eyecatcher
used to identify containers within a task cgroup file.
So this patch, with little extra cost, will enable the
libprocps lxc_containers() guy to handle both strings.
[ additionally, I was shocked to find lxc allows the ]
[ eyecatcher to be changed at ./configure time. such ]
[ a provision has always existed. unfortunately, the ]
[ changed value was only available to root, assuming ]
[ one wished to tackle that undocumented liblxc api. ]
Reference(s):
. what prompted lxc support reevaluation
https://gitlab.com/procps-ng/procps/merge_requests/82
. original lxc support introduced
commit 0557504f9c
Signed-off-by: Jim Warner <james.warner@comcast.net>
A lack of documentation seems to be the major obstacle
to releasing this new library. So, in an effort to get
the ball rolling again, this patch adds the origins of
each item as a comment to six of the new header files.
However, before reviewing how such changes may benefit
that documentation objective, it seemed appropriate to
first reflect on newlib's background & current status.
___________________________________________ BACKGROUND
Discussions about and work on a new library began back
in July 2012 but quickly died. After a lull of 2 years
those discussions were resumed in August 2014 but soon
died also (and no code survived the gitorious demise).
With those early discussions, the recommended approach
was to encapsulate all of the libprocps data offerings
in individual functions. When it came to extensibility
it was suggested we should rely on symbols versioning.
Unfortunately that approach would have made for a huge
Application Programming Interface virtually impossible
to master or even document. And, runtime call overhead
would have been substantial for ps and especially top.
So, an alternative design was sought but there were no
new suggestions/contributions via freelists or gitlab.
Thus, in spite of a lack of library design experience,
the procps-ng team (Craig & Jim) set out to develop an
alternative API, more concise and with lower overhead.
Reference(s):
. 07/01/2012, begin library design discussion
https://www.freelists.org/post/procps/Old-library-calls
. 08/12/2014, revival of library design discussion
https://www.freelists.org/post/procps/libprocs-redesign
_____________________________________ DESIGN EVOLUTION
Our newlib branch first appeared on June 14, 2015. And
our current API actually represents the 4th generation
during the past 3 years of evolution. First, there was
a basic 'new', 'get' and 'unref' approach, using enums
to minimize the proliferation of 'get' function calls.
Then, in anticipation of other programs like ps, where
multiple fields times multiple processes would greatly
increase the number of 'get' function calls, a concept
of 'chains' was introduced. This became generation #2.
Such 'chains' proved unnecessarily complex so 'stacks'
replaced them. This was considered the 3rd generation,
but too many implementation details were still exposed
requiring those users to 'alloc', 'read', 'fill', etc.
Finally, a 4th generation emerged representing several
refinements to standardize and minimize those exported
functions, thus hiding all implementation details from
the users. Lastly, handling of 'errno' was normalized.
Reference(s):
. 06/14/2015, revival of new API discussion
https://www.freelists.org/post/procps/The-library-API-again
. 06/24/2015, birth of the newlib branch
https://www.freelists.org/post/procps/new-library
. 06/29/2015, 2nd generation introduced 'chains'
https://www.freelists.org/post/procps/new-library,8
. 07/22/2015, 3rd generation introduced 'stacks'
https://www.freelists.org/post/procps/newlib-stacks-vs-chains
. 06/18/2016, 4th generation refinements begin
https://www.freelists.org/post/procps/newlib-generation-35
. 11/10/2017, 4th generation standardized 'errno'
https://www.freelists.org/post/procps/some-more-master-newlib-stuff
_______________________________________ CURRENT DESIGN
Central to this new design is a simple 'result' struct
reflecting an item plus its value (thanks to a union).
As a user option, these item structures can be grouped
into 'stacks', yielding many results with just 1 call.
Such a 'stack' can be seen as a variable length record
whose content/order is determined solely by the users.
Within that 'result' structure, the union has standard
C language types so there is never a doubt how a value
should be used in a printf statement. Given that linux
requires a least a 32-bit platform the only difference
in capacity surrounds 'long' integers. And, where such
types might be used, the 32-bit maximums are adequate.
The items themselves are simply enumerators defined in
the respective header files. A user can name any items
of interest then the library magically provides result
structure(s). The approach was proven to be extensible
without breaking the ABI (in commit referenced below).
The 6 major APIs each provide for the following calls:
. 'new' ---------> always required as the first call .
. 'ref' -------------------------> strictly optional .
. 'unref' --------> optional, if ill-behaved program .
. 'get' --------------------> retrieve a single item .
. 'select' ----------------> retrieve multiple items .
And the 'get' and 'select' functions provide for delta
results representing the difference between successive
get/select calls (or a 'new' then 'get/select' call).
For the <diskstats>, <pids>, <slabinfo> & <stat> APIs,
where results are unpredictable, a 'reap' function can
return multiple result structures for multiple stacks.
The <pids> API differs from others in that those items
of interest must be provided at 'new' or 'reset' time,
a function unique to this API. And the <pids> 'select'
function requires PIDs or UIDs which are to be fetched
which then operates as a subset of 'reap'. Lastly, the
'get' function is an iterator for successive PIDs/TIDs
returning items previously identified via 'new/reset'.
To provide assistance to users during development, the
special header 'proc/xtra-procps-debug.h' is available
to check type usage against library expectations. That
check is activated by including this header explicitly
or via build using: ./configure '-DXTRA_PROCPS_DEBUG'.
Reference(s):
. 08/05/2016, type validation introduced
https://www.freelists.org/post/procps/newlib-types-validation
commit e3270d463d
. 08/11/2016, extensibility while preserving ABI example
https://www.freelists.org/post/procps/new-meminfo-fields
commit 09e1886c9e
_________________________ INITIAL DOCUMENTATION EFFORT
The initial attempt, referenced below, dealt primarily
with the <pids> interface. Separate man pages for each
exported function were created. Plus there was another
document describing the items, among other miscellany.
Adopting such an approach encounters several problems:
1. In order to use these man pages, users are required
to already know how to use the library. Or alternately
one could randomly search each of them while trying to
ascertain which function call satisfies their need and
what exactly was the proper compliment/order required.
2. While we can explain what all of those <pids> items
represent, that certainly isn't true for all the APIs.
See the gaps in kernel documentation for <meminfo> and
complete lack of documentation with that <vmstat> API.
3. Our documentation effort should take pains to avoid
unnecessary implementation details. Here's an example:
. "The pointer to info will have memory"
. "allocated and a structure created."
Alternatively, the following conveys user requirements
while not offering any internal implementation detail:
. "You must provide the address of a NULL"
. "info structure pointer."
Reference(s):
. 01/04/2017, initial documentation offering
https://www.freelists.org/post/procps/Using-reap-and-get
commit 2598e9f2ce
___________________ RECOMMENDED DOCUMENTATION APPROACH
I recommend that the newlib documentation consist of 3
man pages only. The first would cover the 5 major APIs
and their common functions. The second would deal with
the <pids> API exclusively, explaining how it differs.
Any remaining exported libproc functions which are yet
to be included could be represented in a 3rd document.
For these new documents the following are are assumed:
1. Since we will not be able to document all items, we
shouldn't try to document any items. We should instead
rely on proc(5) or Documentation/filesystems/proc.txt.
2. Program development often involves referencing some
header file(s). So, make that an absolute requirement.
3. With the addition of item origins, represented with
this commit, and considering that 'types' were already
present, the header file might be all some users need.
4. And who knows, when a user of our libproc complains
about gaps in their documentation, it might prompt the
kernel folks to correct those long standing omissions.
To summarize, I suggest that we replace that libproc.3
document with a more general one explaining the basics
of accessing this new library and the common calls for
most of the major interfaces. We can then create a new
document (libproc-pids.3?), which explains differences
in using the <PIDS> application programming interface.
A final document (libproc-misc.3?) covers what's left.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This change is being made in anticipation of adding the
source origin of each item to the <pids.h> header file.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch will bring the <meminfo> API into line with
that proc(5) document. There were several undocumented
fields that were not noted and these two were omitted:
. 'MmapCopy' was conditional on the #define CONFIG_MMU
. 'Quicklists' depends on the #define CONFIG_QUICKLIST
And we're about to get the following new field in 4.20
which will be represented, at least, in that proc.txt:
. 'KReclaimable' will include SReclaimable plus others
Signed-off-by: Jim Warner <james.warner@comcast.net>
While setting the size of that Hide_pid array to equal
total pids high water mark was probably safe, in truth
there is no real relationship. At some point one could
exceed that HWM if the 'v' toggle was used extensively
and at least 1 of those entries remained non-negative.
This commit simply divorces Hide_tot from the pids HWM
and bases Hide_pid array size on actual run-time need.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Currently, except for tasks that have no parents, when
a process' children are collapsed the '+' indicator is
shown in the first position within that COMMAND field.
This commit simply provides for indenting the '+' char
so it displays next to that program name/command line.
Signed-off-by: Jim Warner <james.warner@comcast.net>
In that commit referenced below, a few edge cases were
addressed regarding vertical positioning involving any
'hidden' tasks. But, 2 additional edge cases remained.
In a running top, if the user employed 'other filters'
(o/O) or 'user filters' (u/U) proper vertical position
was not ensured. And, while this could be easily fixed
by striking the home/end or up/down arrow keys, it was
very poor etiquette to shift this burden to the users.
So, this patch plugs that gap, automating the process.
Reference(s):
commit 9d59ddc466
Signed-off-by: Jim Warner <james.warner@comcast.net>
While setting the size of that Hide_pid array to equal
total pids high water mark was probably safe, in truth
there is no real relationship. At some point one could
exceed that HWM if the 'v' toggle was used extensively
and at least 1 of those entries remained non-negative.
This commit simply divorces Hide_tot from the pids HWM
and bases Hide_pid array size on actual run-time need.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Currently, except for tasks that have no parents, when
a process' children are collapsed the '+' indicator is
shown in the first position within that COMMAND field.
This commit simply provides for indenting the '+' char
so it displays next to that program name/command line.
Signed-off-by: Jim Warner <james.warner@comcast.net>
In that commit referenced below, a few edge cases were
addressed regarding vertical positioning involving any
'hidden' tasks. But, 2 additional edge cases remained.
In a running top, if the user employed 'other filters'
(o/O) or 'user filters' (u/U) proper vertical position
was not ensured. And, while this could be easily fixed
by striking the home/end or up/down arrow keys, it was
very poor etiquette to shift this burden to the users.
So, this patch plugs that gap, automating the process.
Reference(s):
commit c6e68e2fed
Signed-off-by: Jim Warner <james.warner@comcast.net>
From the outset, top has tried to provide some minimal
garbage collection in support of forest view collapse.
For example, with every 'v' keystroke, a check is made
of the currently targeted pids. If all were negative,
which means expanded, that Hide_pid array was emptied.
Recently, yet another efficiency was added wherein the
continuing scan for a targeted pid was terminated when
a match was found. But, one more inefficiency existed.
When a task which was subject to collapse under forest
view mode has disappeared (ended), repeatedly scanning
for such a pid with each iteration makes little sense.
So this commit will negate such targeted pids and thus
avoid scanning every current task looking for a match.
Then, if 'v' is ever stuck at some point in the future
there will be a chance to empty that Hide_pid[] array.
[ hopefully this will be a final tweak of the forest ]
[ view collapse stuff, but cross your fingers anyway ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Our newlib branch has already dropped support for such
old kernels. However, the master branch still supports
them. So this patch will correct a broken #define that
is used to influence the top Summary Area information.
Signed-off-by: Jim Warner <james.warner@comcast.net>
That prior patch set the stage for eliminating the use
of that 'procps.h' header, while retaining support for
a ./configure -DXTRA_PROCPS_DEBUG' during development.
This commit just eliminates top's use of 'procps.h' in
favor of a separate include for needed newlib headers.
Signed-off-by: Jim Warner <james.warner@comcast.net>
If we ever were to eliminate the procps.h header file,
as discussed in the thread referenced below, then that
would impair the current XTRA_PROCPS_DEBUG provisions.
The only remaining way to verify result types would be
to explicitly include that <proc/xtra-procps-debug.h>.
So, this commit will once again enable the ./configure
provision for defining the -DXTRA_PROCPS_DEBUG option.
Reference(s):
https://www.freelists.org/post/procps/newlib-Qualys-patches,6
Signed-off-by: Jim Warner <james.warner@comcast.net>
From the outset, top has tried to provide some minimal
garbage collection in support of forest view collapse.
For example, with every 'v' keystroke, a check is made
of the currently targeted pids. If all were negative,
which means expanded, that Hide_pid array was emptied.
Recently, yet another efficiency was added wherein the
continuing scan for a targeted pid was terminated when
a match was found. But, one more inefficiency existed.
When a task which was subject to collapse under forest
view mode has disappeared (ended), repeatedly scanning
for such a pid with each iteration makes little sense.
So this commit will negate such targeted pids and thus
avoid scanning every current task looking for a match.
Then, if 'v' is ever stuck at some point in the future
there will be a chance to empty that Hide_pid[] array.
[ hopefully this will be a final tweak of the forest ]
[ view collapse stuff, but cross your fingers anyway ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
In forest view mode, once a collapsible parent process
and all of its children (if any) have been identified,
there is no longer a need to scan the remaining tasks.
So this patch will just force a new scan for any other
'Hide_pid' entries which might remain to be identified
after a targeted parent has been completely processed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch includes the following miscellaneous stuff:
. ensure 1 space before any '*' ptr sizeof() reference
. explain the rather cryptic 'ioa' guy a little better
Signed-off-by: Jim Warner <james.warner@comcast.net>
The top program has always specified the maximum stack
depth at 'new' time. Then, in those stacks the minimum
number of result structures were used for representing
only fields actually being displayed in the 4 windows.
That, however, complicated all subsequent access since
each field's enumerator then had to be translated into
a relative position when interacting with the library.
This was accomplished by that Fieldstab 'erel' member.
So this patch eliminates an extra level of indirection
by fully exploiting the existing maximum sized stacks.
Now, the enumerators that top uses to represent fields
also represent their relative positions in each stack.
[ for fields not actually displayed, the position in ]
[ a stack is represented by the 'PIDS_extra' struct. ]
[ thus, there isn't any real library costs for those ]
[ enumerators/fields which aren't currently visible. ]
Signed-off-by: Jim Warner <james.warner@comcast.net>
Previous versions of ps used to only match on the first 15 characters
because that's what the kernel used to provide. Newer kernels have a
longer length for this field so procps has been updated to suit.
References:
procps-ng/procps#101https://bugzilla.suse.com/show_bug.cgi?id=1099091
In forest view mode, once a collapsible parent process
and all of its children (if any) have been identified,
there is no longer a need to scan the remaining tasks.
So this patch will just force a new scan for any other
'Hide_pid' entries which might remain to be identified
after a targeted parent has been completely processed.
Signed-off-by: Jim Warner <james.warner@comcast.net>
This patch includes the following miscellaneous stuff:
. ensure 1 space before any '*' ptr sizeof() reference
. explain the rather cryptic 'ioa' guy a little better
Signed-off-by: Jim Warner <james.warner@comcast.net>
Previous versions of ps used to only match on the first 15 characters
because that's what the kernel used to provide. Newer kernels have a
longer length for this field so procps has been updated to suit.
References:
procps-ng/procps#101https://bugzilla.suse.com/show_bug.cgi?id=1099091
Currently, it isn't possible to establish an 'Inspect'
pipe that relies on SIGINT to end. That's because this
signal will also end the parent process (top) as well.
So this patch will temporarily ignore that signal when
processing any 'Inspect' pipe, allowing one like this:
. pipe ^I Trace Calls ^I /usr/bin/strace -r -p %d 2>&1
Signed-off-by: Jim Warner <james.warner@comcast.net>
Upon startup there exists the potential for some minor
memory leakage should some rcfile 'Inspect' entries be
invalid. By delaying any malloc/strdup until after the
entries are completely validated we will prevent that.
Signed-off-by: Jim Warner <james.warner@comcast.net>
Parent tasks with collapsed children should have their
cpu reflect any unseen tasks only under the following:
1) When built without TREE_VCPUOFF having been defined
2) Exclusively when 'Show_FOREST' display mode was set
3) And only under the current window when in alternate
display mode (except if TREE_VWINALL has been defined)
So, this commit just ensures these objectives are met.
Reference(s):
. issue that began odyssey
https://gitlab.com/procps-ng/procps/issues/99
. original cpu implementation
commit 7c7903e50d
Signed-off-by: Jim Warner <james.warner@comcast.net>
Currently, it isn't possible to establish an 'Inspect'
pipe that relies on SIGINT to end. That's because this
signal will also end the parent process (top) as well.
So this patch will temporarily ignore that signal when
processing any 'Inspect' pipe, allowing one like this:
. pipe ^I Trace Calls ^I /usr/bin/strace -r -p %d 2>&1
Signed-off-by: Jim Warner <james.warner@comcast.net>
