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" \ ===
Maintainer feedback, please!
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.
OP: Please attach a patch and I will consider it.
Created attachment 206262 [details] svn-diff-bsdstats I take it from comment1. Not tested.
Comment on attachment 206262 [details] svn-diff-bsdstats Uups, there is a error in.
(In reply to scrappy from comment #0) Does not work -> "Syntax error: end of file unexpected (expecting ";;")" EOT is problematic.
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 ...
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.
(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 :)
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 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).
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 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.
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.
Assign bug to myself as I agreed with yuri@ by e-mail to take maintainership of sysutils/bsdstats.
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
Commited, thanks!