Bug 226899 - sysutils/p5-BSD-Sysctl: devstat broken [PATCH]
Summary: sysutils/p5-BSD-Sysctl: devstat broken [PATCH]
Status: Open
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Kurt Jaeger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-24 21:52 UTC by Helge Oldach
Modified: 2019-02-11 21:19 UTC (History)
4 users (show)

See Also:
pi: maintainer-feedback-


Attachments
BSD-Sysctl-0.11-hmo.diff (6.08 KB, text/plain)
2018-03-24 21:52 UTC, Helge Oldach
no flags Details
replacement for files/patch-Sysctl.xs (merged current and proposed patch) (9.88 KB, patch)
2018-05-11 09:11 UTC, Helge Oldach
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Helge Oldach 2018-03-24 21:52:36 UTC
Created attachment 191790 [details]
BSD-Sysctl-0.11-hmo.diff

Access to kern.devstat.all is not correctly implemented. The attached patch fixes it and also adds access to a few more interesting values. Tested on 11-STABLE both i386 and amd64.
Comment 1 Kurt Jaeger freebsd_committer 2018-05-09 19:47:26 UTC
Can you explain your patch ? Is it a patch for files/patch-Sysctl.xs ?
Or does it replace that file ? Or is it a second patch to
work/BSD-Sysctl-0.11/Sysctl.xs ?

Thanks!
Comment 2 Kurt Jaeger freebsd_committer 2018-05-09 19:48:26 UTC
And: have you reported the patch on CPAN ?
Comment 3 Helge Oldach 2018-05-10 09:29:47 UTC
(In reply to Kurt Jaeger from comment #1)
sysctl("kern.devstat.all") is not strikingly useful at present: It is delivering just the first element of struct devstat, and only a very limited number of sub-elements thereof. This patch extends it to deal with the devstat "header" properly and deliver all devstat elements as separate hash elements.

Without patch:

# perl -mBSD::Sysctl=sysctl -e 'print join " ", keys %{sysctl("kern.devstat.all")}, "\n"'
allocated sequence endcount unitno busyfromfrac devno busyfromsec startcount
# 

Patched:

# perl -mBSD::Sysctl=sysctl -e 'print join " ", keys %{sysctl("kern.devstat.all")}, "\n"'
0.devno 0.operations_write 0.allocated 0.endcount 0.operations_free 1.operations_write 1.busyfromfrac 1.devno 1.operations_read 1.device_name 0.device_name 0.bytes_free 1.unitno 1.allocated 0.bytes_write 0.sequence 1.bytes_read 0.bytes_read 1.busyfromsec 1.operations_free 0.unitno 0.busyfromsec 0.bytes_no_data 0.operations_no_data 0.operations_read 0.startcount 1.bytes_free 0.busyfromfrac 1.endcount 1.bytes_no_data 1.operations_no_data 1.bytes_write 1.sequence 1.startcount
# 

"0." indicates the first struct devstat element, "1." the second, and so forth. In theory this should better be returned through an array but that would impact the BSD::Sysctl semantics heavily, so I preferred this somewhat hackish approach.

Also:

# perl -mBSD::Sysctl=sysctl -e 'map { print $_, " " } grep { s/\..*// } grep { /.*\.device_name/ } keys %{sysctl("kern.devstat.all")}; print "\n"'
1 0
# 

Indeed it's a second patch to Sysctl.xs (visible in the diff).
Comment 4 Helge Oldach 2018-05-10 09:30:56 UTC
(In reply to Kurt Jaeger from comment #2)
No, I wasn't sure what the "leading" repository is. As this module is very FreeBSD specific I was assuming it's better suited here.
Comment 5 Helge Oldach 2018-05-10 10:13:25 UTC
(In reply to Helge Oldach from comment #3)
To add, without patch the values returned by sysctl("kern.devstat.all") are simply wrong because of an index error when accessing buf. This is also fixed by the patch (by having struct devstat correctly pointing to buf + sizeof(buf)).

For example, unpatched:

# perl -mBSD::Sysctl=sysctl -e '%devstat = %{sysctl("kern.devstat.all")}; print "devno: ", $devstat{"devno"}, "\n"'
devno: 72302592
# 

This devno value is clearly nonsense. Similar garbage if you query different elements.

Patched:

# perl -mBSD::Sysctl=sysctl -e '%devstat = %{sysctl("kern.devstat.all")}; print "devno: ", $devstat{"0.devno"}, "\n"'
devno: 1
#
Comment 6 Kurt Jaeger freebsd_committer 2018-05-11 06:51:05 UTC
Thanks for the details -- I still do not understand if your patch replaces files/patch-Sysctl.xs or if we need to merge both patches ?
Comment 7 Helge Oldach 2018-05-11 08:18:09 UTC
(In reply to Kurt Jaeger from comment #6)
Both patches can just be merged into one. No overlap, just a line numbering offset. Shall I submit a patch to files/patch-Sysctl.xs instead?
Comment 8 Kurt Jaeger freebsd_committer 2018-05-11 08:20:00 UTC
yes, a patch like that would be easier to integrate.
Comment 9 Helge Oldach 2018-05-11 09:11:23 UTC
Created attachment 193275 [details]
replacement for files/patch-Sysctl.xs (merged current and proposed patch)
Comment 10 Helge Oldach 2018-05-11 09:34:41 UTC
Comment on attachment 193275 [details]
replacement for files/patch-Sysctl.xs (merged current and proposed patch)

Sorry - of course this is a *patch* to files/patch-Sysctl.xs, as suggested by pi@. Sorry for botching up the description.
Comment 11 Kurt Jaeger freebsd_committer 2018-05-11 09:43:56 UTC
Ok, it builds on 12a, 11.1a, 10.4i, but:

#!/usr/local/bin/perl

use BSD::Sysctl 'sysctl';

%devstat = %{sysctl("kern.devstat.all")};
print "devno: ", $devstat{"0.devno"}, "\n";

says:
devno: 0

Both on 12a and 11.1a. What am I doing wrong ?
Comment 12 Kurt Jaeger freebsd_committer 2018-05-11 09:45:28 UTC
On 12a:

kern.devstat.version: 6
kern.devstat.generation: 126
kern.devstat.numdevs: 5

On 11.1a:

kern.devstat.version: 6
kern.devstat.generation: 262
kern.devstat.numdevs: 3

So it should list something, no ? Do I need to load some kernel module or... ?
Comment 13 Helge Oldach 2018-05-11 10:59:21 UTC
(In reply to Kurt Jaeger from comment #11)
Nothing is wrong. :-)

The first entry in struct devstat just has devno==0 on your system. On mine, just devno==1. devno is just a sequential number in the devstat table.


Here's a more useful example that you might try (the output designed to be fed into RRDtool):

#!/usr/local/bin/perl

use BSD::Sysctl 'sysctl';

%devstat = %{sysctl("kern.devstat.all")};

foreach my $x (grep { s/\..*// } grep { /.*\.device_name/ } keys %devstat) {
        print $devstat{"$x.device_name"}, $devstat{"$x.unitno"}, " ", join(":", map { $devstat{"$x.$_"} } qw/bytes_read bytes_write bytes_free operations_read operations_write operations_free operations_no_data/), "\n";
}


Output on my Intel NUC:

pass0 21504:0:0:42:0:0:14
ada0 26650459136:83211827712:0:2444761:2213351:0:2


and one a system with a few more devices:

ada0 80609812480:135090176512:0:5059803:4089766:0:0
pass0 23040:0:0:45:0:0:15
cd0 0:0:0:4:0:0:8
ada1 17531804160:44417896960:0:2994409:1148838:0:0
pass1 23040:0:0:45:0:0:15
pass2 0:0:0:0:0:0:0
Comment 14 Helge Oldach 2018-05-11 11:28:24 UTC
(In reply to Helge Oldach from comment #13)
Another example:

#!/usr/local/bin/perl

use BSD::Sysctl 'sysctl';

%devstat = %{sysctl("kern.devstat.all")};

foreach my $x (sort grep { s/\..*// } grep { /.*\.device_name/ } keys %devstat) {
        print "x=$x devno=", $devstat{"$x.devno"}, " device=", $devstat{"$x.device_name"}, $devstat{"$x.unitno"}, "\n";
}


Output on a device with a number of devices:

x=0 devno=4 device=ada0
x=1 devno=5 device=ada1
x=2 devno=3 device=cd0
x=3 devno=0 device=pass0
x=4 devno=1 device=pass1
x=5 devno=2 device=pass2
Comment 15 Helge Oldach 2018-05-11 11:32:28 UTC
(In reply to Helge Oldach from comment #14)
To complete the picture, here's what sysctl ker.devstat says on that particular system:

kern.devstat.version: 6
kern.devstat.generation: 958
kern.devstat.numdevs: 6

As expected, 6 entries in struct devstat. :-)
Comment 16 Kurt Jaeger freebsd_committer 2018-05-11 11:50:24 UTC
Committed, thanks!
Comment 17 Kurt Jaeger freebsd_committer 2018-05-11 11:50:36 UTC
TODO: submit patch upstream
Comment 18 commit-hook freebsd_committer 2018-05-11 11:50:52 UTC
A commit references this bug:

Author: pi
Date: Fri May 11 11:50:17 UTC 2018
New revision: 469629
URL: https://svnweb.freebsd.org/changeset/ports/469629

Log:
  sysutils/p5-BSD-Sysctl: fix access to kern.devstat

  - see the PR for a few nice examples on how to use it

  PR:		226899
  Submitted by:	Helge Oldach <freebsd@oldach.net>
  Approved by:	david@landgren.net (maintainer timeout)

Changes:
  head/sysutils/p5-BSD-Sysctl/Makefile
  head/sysutils/p5-BSD-Sysctl/files/patch-Sysctl.xs
Comment 19 Walter Schwarzenfeld freebsd_triage 2019-02-11 18:54:24 UTC
Is here anything to do, or could we close this?
Comment 20 Kurt Jaeger freebsd_committer 2019-02-11 21:19:37 UTC
The patch still needs to be submitted and accepted upstream