Bug 229347 - sysutils/bsdstats: Device reporting broken
Summary: sysutils/bsdstats: Device reporting broken
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Lorenzo Salvadore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-26 15:51 UTC by scrappy
Modified: 2020-03-01 13:28 UTC (History)
3 users (show)

See Also:
yuri: maintainer-feedback+


Attachments
svn-diff-bsdstats (1.19 KB, patch)
2019-08-04 04:12 UTC, Walter Schwarzenfeld
no flags Details | Diff
patch as attachment instead of cut-n-paste (1.39 KB, patch)
2019-09-01 09:06 UTC, scrappy
no flags Details | Diff
tested patch in svn diff format (1.25 KB, patch)
2020-01-15 15:37 UTC, Lorenzo Salvadore
no flags Details | Diff
Fix for changes to pciconf -l in FreeBSD 13-CURRENT (1.78 KB, patch)
2020-01-17 04:24 UTC, scrappy
no flags Details | Diff
Fix Device reporting, and add code to allow for ports reporting on MidnightBSD (3.49 KB, patch)
2020-01-31 21:41 UTC, scrappy
scrappy: maintainer-approval? (yuri)
Details | Diff
bsdstats -- patch + maintainership transfer (3.78 KB, patch)
2020-02-28 22:29 UTC, Lorenzo Salvadore
salvadore: maintainer-approval? (yuri)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description scrappy 2018-06-26 15:51:10 UTC
Incorrect usage of a for loop instead of while causes the query_string to be mis-generated, causing device reports to be unparseable on the server end ... following patch needs to be applied to fix ...

===
*** /tmp/300.statistics	Mon Jun 25 04:17:01 2018
--- /usr/local/etc/periodic/monthly/300.statistics	Mon Jun 25 04:28:00 2018
***************
*** 354,365 ****
     FreeBSD|DragonFly)
       local query_string=""
       local line
!       for line in $(${PCICONF} -l); do
         local DRIVER=$(echo "${line}" | ${AWK} -F\@ '{print $1}')
         local DEV=$(echo "${line}" | ${AWK} '{print $4}' | ${CUT} -c8-15)
         local CLASS=$(echo "${line}" | ${AWK} '{print $2}' | ${CUT} -c9-14)
         query_string=$query_string`echo \&dev[]=${DRIVER}:${DEV}:${CLASS}`
!       done

       echo_begin "Posting device statistics to ${checkin_server_description}"
       do_http_request_check_status "GET" "/scripts/report_devices.php?token=${TOKEN}&key=${KEY}$query_string" \
--- 354,368 ----
     FreeBSD|DragonFly)
       local query_string=""
       local line
!       while read line
!       do
         local DRIVER=$(echo "${line}" | ${AWK} -F\@ '{print $1}')
         local DEV=$(echo "${line}" | ${AWK} '{print $4}' | ${CUT} -c8-15)
         local CLASS=$(echo "${line}" | ${AWK} '{print $2}' | ${CUT} -c9-14)
         query_string=$query_string`echo \&dev[]=${DRIVER}:${DEV}:${CLASS}`
!       done << EOT
! $(${PCICONF} -l)
! EOT

       echo_begin "Posting device statistics to ${checkin_server_description}"
       do_http_request_check_status "GET" "/scripts/report_devices.php?token=${TOKEN}&key=${KEY}$query_string" \
===
Comment 1 Walter Schwarzenfeld freebsd_triage 2019-08-03 07:58:29 UTC
Maintainer feedback, please!
Comment 2 Yuri Victorovich freebsd_committer 2019-08-03 08:47:33 UTC
Patches are welcome. -)

But I think that bsdstats has never worked well in a sense that its stats are based on a very small dataset and aren't too meaningful. Not enough people have it installed.
Comment 3 Yuri Victorovich freebsd_committer 2019-08-04 03:59:06 UTC
OP: Please attach a patch and I will consider it.
Comment 4 Walter Schwarzenfeld freebsd_triage 2019-08-04 04:12:41 UTC
Created attachment 206262 [details]
svn-diff-bsdstats

I take it from comment1. Not tested.
Comment 5 Walter Schwarzenfeld freebsd_triage 2019-08-04 04:23:37 UTC
Comment on attachment 206262 [details]
svn-diff-bsdstats

Uups, there is a error in.
Comment 6 Walter Schwarzenfeld freebsd_triage 2019-08-04 05:58:30 UTC
(In reply to scrappy from comment #0)
Does not work ->
"Syntax error: end of file unexpected (expecting ";;")"

EOT is problematic.
Comment 7 scrappy 2019-09-01 09:06:20 UTC
Created attachment 207059 [details]
patch as attachment instead of cut-n-paste

I just tested patch here again, to make sure, and it seems to work fine ... sample code:

==
#!/bin/sh -
#

# Standard commands used here
#
PCICONF=/usr/sbin/pciconf
AWK=/usr/bin/awk
CUT=/usr/bin/cut

       while read line
       do
         DRIVER=$(echo "${line}" | ${AWK} -F\@ '{print $1}')
         DEV=$(echo "${line}" | ${AWK} '{print $4}' | ${CUT} -c8-15)
         CLASS=$(echo "${line}" | ${AWK} '{print $2}' | ${CUT} -c9-14)
         query_string=$query_string`echo \&dev[]=${DRIVER}:${DEV}:${CLASS}`
       done << EOT
$(${PCICONF} -l)
EOT

echo $query_string
==

Output is as expected:

==
&dev[]=none0:2f818086:088000&dev[]=none1:2f368086:110100&dev[]=none2:2f378086:110100
==

Tested on:

==
FreeBSD 11.3-PRERELEASE #14 r349760: Fri Jul  5 08:58:00 UTC 2019
==

I've attached ( vs cut-n-paste ) a diff -c on the changeg ... maybe something didn'tcome through properly in my original ...
Comment 8 Lorenzo Salvadore freebsd_committer 2020-01-15 15:37:52 UTC
Created attachment 210761 [details]
tested patch in svn diff format

I repropose the patch in svn diff format.
It is very similar to the one that was already submitted, but it removes a space preceding the second EOT that was responsible for the reported error.
Now it works.

I tested successfully the patched port on 13.0-CURRENT.
Comment 9 scrappy 2020-01-16 22:54:08 UTC
(In reply to Lorenzo Salvadore from comment #8)

I have a new patch for this that extends support for reporting FreeBSD 13.x devices .. the format of pciconf -l appear to have changed between 12.x and 13.x, with 12.x looking like:

hostb0@pci0:0:0:0:      class=0x060000 card=0x12371d0f chip=0x12378086 rev=0x00 hdr=0x00

and 13.x looking like:

hostb0@pci0:0:0:0:	class=0x060000 rev=0x02 hdr=0x00 vendor=0x8086 device=0x1237 subvendor=0x0000 subdevice=0x0000

I sent it over to Lorenzo to review and will let him post the updated patch, just to save some time :)
Comment 10 scrappy 2020-01-17 04:24:14 UTC
Created attachment 210806 [details]
Fix for changes to pciconf -l in FreeBSD 13-CURRENT

The attached patch is the output of 'svnlite diff .' in the sysutils/bsdstats directory.

The patch:
  - removes PORTREVISION
  - Increases PORTVERSION to 6.1
  - rewrites the report_device logic to handle a change in pciconf -l output that exists in FreeBSD 13.x 
  - changes to using /usr/bin/nc on DragonFly BSD

Since DragonFly and TrueOS both appear to use our ports system, I have made sure to test the changes on both of their OSs as well, so that it is verified on:

  - FreeBSD 12.x
  - FreeBSD 13.x
  - DragonFlyBSD 5.6.2
  - TrueOS Unstable ( FreeBSD 13.x, but without the changes to pciconf -l yet )

And I tested reapplying the patch to make sure it works as expected:

==
root@hub:/usr/ports/sysutils/bsdstats # cd /usr/ports/sysutils
root@hub:/usr/ports/sysutils/bsdstats # rm -rf bsdstats
root@hub:/usr/ports/sysutils/bsdstats # svnlite update
root@hub:/usr/ports/sysutils/bsdstats # patch < /tmp/bsdstats.svn.diff
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: Makefile
|===================================================================
|--- Makefile   (revision 521979)
|+++ Makefile   (working copy)
--------------------------
Patching file Makefile using Plan A...
Hunk #1 succeeded at 2 with fuzz 1.
Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: files/300.statistics.in
|===================================================================
|--- files/300.statistics.in    (revision 521979)
|+++ files/300.statistics.in    (working copy)
--------------------------
Patching file files/300.statistics.in using Plan A...
Hunk #1 succeeded at 42.
Hunk #2 succeeded at 354.
done
==

Hopefully I've covered all the bases ...

Thanks ...
Comment 11 Lorenzo Salvadore freebsd_committer 2020-01-17 12:15:53 UTC
Comment on attachment 210806 [details]
Fix for changes to pciconf -l in FreeBSD 13-CURRENT

scrappy:

Please remember to ask for maintainer approval when you create a new patch that you want to be merged (I also forgot on my patch, but it is now obsolete, so it does not matter). You just have to set the "maintainer-approval" flag to "?" and the "requestee" field to the mail address of the maintainer. I did it for you for the actual patch.
I tested (successfully) only this patch. I saw it is different from the last one I received by mail: I assume the one loaded on bugzilla to be always the most up to date. If it is not, please mark the one on bugzilla as obsolete and on bugzilla upload the new one: I will automatically receive a mail that will warn me about the new patch (I am in the CC list of the bug report). If I have patches on bugzilla and patches in my mail box I get confused and I do not know any more which one I should test.

yuri:

I have tested successfully the patch on 13.0-CURRENT r356358.
I see you maintain plenty of ports (999 according to portscout!) and bsdstats seems a bit confusing, at least at the moment. Since I just joined scrappy in working on the bsdstats site and software, I think it would make more sense and it would be easier for all of us if I maintained the port: then, if it is fine for you, I would like to adopt this port.
Please keep in mind that, although I am marked as a committer on bugzilla, I am not. So the help of a committer, such as yourself, would still be needed even if you agree to the maintainership transfer (but of course committing a patch marked with "maintainer-approval = +" should be easier and quickier).
Comment 12 scrappy 2020-01-31 21:41:02 UTC
Created attachment 211228 [details]
Fix Device reporting, and add code to allow for ports reporting on MidnightBSD

This patch is an extension of the previous ones, and includes:

  - fix for new output format of pciconf -l in 13-CURRENT
  - fix for device reporting ( switch from a for loop to a while loop )
  - add support for ports reporting from MidnightBSD systems ( they use our ports tree )
Comment 13 Lorenzo Salvadore freebsd_committer 2020-02-01 11:36:32 UTC
Comment on attachment 211228 [details]
Fix Device reporting, and add code to allow for ports reporting on MidnightBSD

I tested successfully the patch on 13-CURRENT r357262.
Comment 14 Lorenzo Salvadore freebsd_committer 2020-02-28 22:29:21 UTC
Created attachment 212035 [details]
bsdstats -- patch + maintainership transfer

This is the same patch scrappy submitted to which I added the maintainership transfer I proposed in comment #11.

By the way yuri, I am now a committer too, so if you agree to the maintainership transfer I could maintain the port including the commits.
Comment 15 Lorenzo Salvadore freebsd_committer 2020-02-29 20:03:29 UTC
Assign bug to myself as I agreed with yuri@ by e-mail to take maintainership of sysutils/bsdstats.
Comment 16 commit-hook freebsd_committer 2020-03-01 13:27:37 UTC
A commit references this bug:

Author: salvadore
Date: Sun Mar  1 13:27:22 UTC 2020
New revision: 527552
URL: https://svnweb.freebsd.org/changeset/ports/527552

Log:
  Update sysutils/bsdstats to 6.2

  - Fix files/300.statistics.in so that it can work with the new output
    format of pciconf -l in 13.0-CURRENT.
  - Fix a bug in device reporting.
  - Add support for ports reporting from MidnightBSD systems, whose ports
    tree is based on FreeBSD ports tree.
  - Changes to using /usr/bin/nc on DragonFly BSD, whose ports tree is based
    on FreeBSD ports tree.
  - Assume maintainership of sysutils/bsdstats. [1]

  PR:		229347
  Submitted by:	scrappy
  Approved by:	yuri (maintainer) [1], gerald (mentor)

Changes:
  head/sysutils/bsdstats/Makefile
  head/sysutils/bsdstats/files/300.statistics.in
Comment 17 Lorenzo Salvadore freebsd_committer 2020-03-01 13:28:05 UTC
Commited, thanks!