Add libxo support to pciconf

[se]

Summary

Non-error output is through libxo, with "text" as the default format.
On my system, the output is identical to the previous version, unless a different format is specified.
All err/errx/warn/... calls are changed to xo_... variants - is this OK?
There are several over-long lines and probably other violations of style - I did not want to break long lines to ease the review.
HTML-Output is ugly, but this appears to be the case for other converted utilities as well.
Cleanup of HTML could be performed as a follow-up change, if anybody has a use for it.
I have not found any PCI VPD on my test system, and I could not find any specification details for the data.

Test Plan

This version of pciconf should generate 100% identical output, except
when one of the structured XO formats is requested.

I have tested the following formats: text, xml, html, json.
Text output with no --libxo option or with "--libxo test" was unchanged for all output generated on my test system, but I cannot generate all possible output, thus coverage is not good enough.

Test commands:

pciconf --libxo %format% -lbcevV
pciconf --libxo %format% -lbcevV %selector%
pciconf --libxo %format% -a %selector%
pciconf --libxo %format% -r %selector%

In all cases except -a the selector should be the bus-id of an existing device or the device name (e.g. "re0").
For -a both existing and non-existing devices should be checked, as well as devices matching "none*" in "pciconf -l" output.

[se]

1. Updating D1206: Add libxo support to pciconf #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

Update Makefile (use LIBADD instead of LDADD/DPADD) and pciconf.c
(replace err/warn with equivalent xo_err/xo_warn function calls).

[se]

Update after review by Phil Shafer and extend to cover errors and capabilities

Summary:

This version contains many fixes to issues pointed out by Phil and
a number of further enhancements:

• Use of XO lists and instances to structure the information
• Descriptive identifiers instead of abbreviations
• Addition of XO support to PCI/PCIe error reporting
• Addition of XO support to PCI/PCIe capability information printing

I do not have access to a PCI or PCI-Express Spec. with information on
the capabilities and their specified names, but have only used publicly
available information to select XML/JSON labels. A pointer to a freely
available specification of PCI/PCIe capabilities (just the path that
might help chose good names for data fields) would be highly appreciated.

Test Plan:

This version of pciconf should generate 100% identical output, except
when one of the structured XO formats is requested.

Please test with:
pciconf -lbecv

I do not know whether the labels used for XML amd JSON output of PCI/PCIe
capabilities are well chosen. Several names are very long, but I wanted to
avoid cryptic abbreviations. Suggestions for better or more correct names
are welcome.

I could not test output of Intel or AMD (Hyper Transport) specific
capabilities (my Intel system uses SATA AHCI, but I do not see that in
the reported capabilities; I do not have any FreeBSD systems on AMD
hardware, currently).

I'm adding a few reviewers (jhb because he did the initial commit of cap.c
and kib because of the HT specific information that I cannot test). I hope
you don't mind being selected as potential reviewers.

Reviewers:

[e-mail]
[e-mail]
[e-mail]
[e-mail]

Subscribers:

[kib]

cap.c

62

What is the XML (or similar SGML-like) output for this ? Is this attribute or text element ? Whatever is it, having the list where members are separated by space somewhat defeats the idea of XML.

76–77

Style requires /* */ comments. But the question is, why do you need the old code around at all ?

88

Why is this line commented out ?

[se]

Hi Kostik,

thanks a lot for your review!
I have answered your questions in the sources. I'm not decided what the best structure of the JSON and XML output is for e.g. PCIe power states (D0..D3).

Regards, STefan

cap.c

62

OT question: How do I prevent "D3" from being interpreteted as a link to another review??? It is meant to be plain text ...

Yes, but this is one of the places where I'm not sure, what the best output format is. For a device that e.g. supports "D0", "D1" and "D3":

supports-power-state-d0: "true",
supports-power-state-d1: "true",
supports-power-state-d2: "false",
supports-power-state-d3: "true",

Support for "D0" and "D3" ist not optional - should these lines be ommitted?

Is it better to remove all lines with value "false"?

Then the only output would be:

supports-power-state-d1: "true"

And BTW: I do not know whether JSON needs quotes around "true" and "false" (are these string values or can true and false be used to mean booleans?)

An alternative might be an array of supported power states:

supported-power-states: { [

power-state: "D0", 
power-state: "D1", 
power-state: "D3"

] }

There are further places, where several representations are possible and I'm not sure what queries to expect (which might help choose the most appropriate structure).

76–77

C++-style comments are left in my WIP version of pciconf, since I want to have the original text output around when working on structured output.
All these comments will be stripped before committing to -CURRENT and the form helps me locate them quickly ...
(Arghhh: Is the there any way to have // in the text without it being considered markup???)

88

Only "v3 was printed in the unpatched version and I do not know, whether the system has AGP v1 or v2, if the version is "<3". So I decided to omit the version value, if I do not know which value to print.

An alternative might be:

agp-version-3: "true"

(and either "false" for version < 3, or this whole element ist omitted - see my other comment on this topic for the power states - I'm looking for opinions and I do not plan to commit XO support before the method that is choosen is applied to all appropriate places).

[se]

Make parameters to xo_open_list() and xo_open_instance() match
and use singular form (requirement pointed out by Phil Shafer).

Fix format strings (errors pointed out by xolint).

[se]

Fix error printing (final xo_close_list() was called to often).

[allanjude]

cap.c

62

One option here is a 'leaf list' with output like:

supported-power-states: [ "D0", "D1", "D3" ],

pciconf.c

110

best practise is now to have a xo_set_version() as the very first field to version the output

[editor_callfortesting.org]

Hello all,

I would like to tickle this review given that any and all pciconf formatting is appreciating for bhyve PCI pass-through.

A brief listing of devices would be great and this review, in theory, provides the plumbing to allow the user to produce their own brief listing.

Is this review still valid and do the concerns raised remain valid?

[editor_callfortesting.org]

In D1206#809386, @editor_callfortesting.org wrote:

Is this review still valid and do the concerns raised remain valid?

That said, going way out on a limb: pciconf does not appear to have default output. Type the command and you get "usage"

How about it provide the PCI ID and description by default with a tab to separate them? i.e.:

vgapci0@pci0:0:2:0 Xeon E3-1200 v2/3rd Gen Core processor Graphics Controller
xhci2@pci0:4:0:0 uPD720202 USB 3.0 Host Controller
...

[se]

This review is old (more than 7 years) and pciconf has been extended since then - the patch needs to be reviewed and extended before it could be applied today.

There was a strong opinion expressed by mail (and not recorded in this review) that pciconf was quite complex and the xo style formatting directives interspersed with logic to fetch the values would make it a lot harder to maintain and to extend the functionality for new PCI/PCIe features.
I think it was Warner Losh who suggested to extract all the logic of pciconf into a library and to use that library to provide the values for representation by libxo.
Such a library could also be accessed from C programs but also from LUA scripts for example, without the need to parse any text format.

I did accept this position as valid, put the splitting of pciconf in a program that prints values returned by a library on my TODO list - but have spent little time on actually attempting the split.
If there is a strong interest, I could still work on separating the library from the program, provide LUA bindings, and extend and update the xo formatting to support the additional information that has been added since 2014.