Date: Sat, 06 Dec 2014 15:34:09 +0100 From: Stefan Esser <se@freebsd.org> To: Phil Shafer <phil@juniper.net> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, "Simon J. Gerraty" <sjg@juniper.net> Subject: Re: [Review] pciconf converted to use libxo [D1206] Message-ID: <548313E1.1040701@freebsd.org> In-Reply-To: <201412042056.sB4KubEC029247@idle.juniper.net> References: <201412042056.sB4KubEC029247@idle.juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Am 04.12.2014 um 21:56 schrieb Phil Shafer: > Stefan Esser writes: >> the following review URL shows my proposed changes to make pciconf >> optionally output XML or JSON formatted data via libxo: >> https://reviews.freebsd.org/D1206 > > Looks good. some nits: Hi Phil, thank you *very* much for your review and the valuable hints! > Remove the call to xo_create(), which creates a new handle. All > your calls are using the default handle, and since you're not saving > the returned handle, it's pointless. OK. > Use "revision" for "rev" and "header" instead of "hdr". See the > notes re: full words in xo_format(5). Yes, I see. I have fixed the names in my local working copy ... (AFAIR, the man-pages did not exist in -CURRENT when I started to work on libxo support for pciconf in early October. I used the information from http://juniper.github.io/libxo/libxo-manual.html as reference. The man-pages look well structured and I appreciate them.) BTW: Any reason that xolint.1 ist not installed? And another BTW: The man-page for xo(1) does only mention xo_emit(3) and lacks reference to other xo man-pages. The same applies to the man-page for xo_create(3), which has "SEE ALSO xo_emit(3) and" and stops there. (I did not check all man-pages for missing references, but many of the others seem to have a reasonable SEE ALSO list.) > The xo_open_list, xo_open_instance, and xo_container functions > are needed to frame the calls to xo_emit. xo_open_list is used > before starting a list, and xo_open_instance before each instance > in that list: > > xo_open_list("device"); > for (p = conf; p < &conf[pc.num_matches]; p++) { > xo_open_instance("device"); > xo_emit(...); > xo_close_instance("device"); > } > xo_close_list("device"); > > These calls are needed to marshall data correctly into the various > output formats. Each xo_open_xxx needs a matching xo_close_xxx > call. I wondered about correct use of the different xo_open_XXX calls. Seems they do not make any difference for XML and HTML, but are important for correct JSON output. > Does that make sense? You'll need to redo these calls in your code. > Please let me know what can be done in the docs to make this more > clear. The xo_open_list man-page seems reasonably clear, I just did not know it existed ;-) > If "device-id" is the key for this list, use the "k" > modifier: > > xo_emit("{k:device-id/%s%d}..."); > > This will allow the use of that key in XPath expressions. This does not seem to work for me, for example: xo_emit("{k:device-id/%s%d}@{:bus-id/pci%d:%d:%d:%d}:\t..." generates e.g. <device> <device-id>re0</device-id> <bus-id>pci0:4:0:0</bus-id> <class-id>0x020000</class-id> <card-id>0x84321043</card-id> <chip-id>0x816810ec</chip-id> <chip-revision>0x06</chip-revision> <header-type>0x00</header-type> </device> which is identical to the output without the "k". What am I doing wrong? > + xo_destroy(NULL); > > You don't need to destroy the default handle. xo_finish is sufficient. OK. > You have: > > + xo_emit(" VPD ident = '{:ident/%s}'\n", pve_str); > > and: > > + xo_emit(" VPD {:mode/ro} {:keyword/CP} = ID {:id/%02x} in map {:addr/0x%x}[{:size/0x%x}]\n", > > I'm not sure what the contents are but it seems like the former > might have "VPD {:mode/indent}" to be consistent. The problem is, that I do not know much about VPD and have no device that has it. I had been hoping for a review of these particular lines from somebody who knows VPD. Guess it's best to directly ask jhb for a review of this part, since he added VPD printing in January ... > - printf("%0*x", width*2, read_config(fd, sel, reg, width)); > + char buf[11]; > + snprintf(buf, sizeof(buf), "%0*x", width*2, read_config(fd, sel, reg, width)); > + xo_emit("{:cfgdata/%s}", buf); > > libxo should be able to handle this directly, without the need for 'buf': The information in the HTML docs on GitHub made me think, that this format was not supported. > xo_emit("{:data/%0*x}", width*2, read_config(fd, sel, reg, width)); This results in better output, for example with JSON: "cfgdata": d001 instead of with the string buffer: "cfgdata": "0000d001" But it appears, that xo_emit adds blanks to the left of the value, while I'd expect the padding to use zeroes. I changed the format to "{:data/0x%0*x}" and now I get "cfgdata": "0x0000d001" <-- xo_emit("{:data/0x%s}", ...) "cfgdata": 0x0000d001 <-- xo_emit("{:data/0x%0*x}", ...) I.e. it seems that the leading zeroes are replaced by blanks, but non-leading zeroes are kept (' d001' vs. '0x0000d001'). Not sure whether that is expected behaviour - at least it differs from printf ... I found a few more nits in the version of pciconf that I uploaded to the reviews server (e.g. with printing of config space data), and the printing of VPD might be broken if different record types are not grouped together (see the xo_close_list() functions that are not strictly matching the corresponding open functions). I'll probably have to split the VPD output into three distinct loops, two for lists (capabilities and items) and one for the remaining record types. It'll take me a few days to prepare a new version and I'm still open for further comments. >> I wanted to see how much effort the conversion of a utility that >> was not written with libxo in mind makes - and I'm pleased by the >> possibility to mostly mechanically replace printf/err/warn calls >> with their libxo counterparts. > > I tried to make it plug compatible. I noticed that and appreciate it. >> The documentation of libxo seems incomplete (e.g. no mention of >> xo_error() that I could find), and some functions do not seem to >> produce the expected result (xo_err() prints the message as given >> in the format string, but does not automatically append strerror() >> as err() does ...). > > Looks like I missed xo_error completely. I'll add docs for it. > > xo_err() should be identical to err(), but it's broken. I'll take > a look. Thanks. >> A FreeBSD man-page that lists all functions and that can be used >> as a reference when libxo principles are already known would be >> really great. ;-) > > A libxo(3) page? I'll cook one up. I have noticed the commit that added lots of MLINKS and expecially the libxo(3) and it is a good introduction to the library. BTW: Another man-page that lacks references: xo(1) mentions xo_emit(3), but should probably point to libxo(3) instead ... >> The HTML-Output looks ugly if directly viewed in a web browser, >> but that's not different than what other utilities converted to >> use libxo produce ... > > I'm working on a utility called "xohtml" that adds a css file to the > output to make it viewable. It doesn't install yet, but you can > run it by hand using something like: OK, that seems like a good addition to the whole package. > sh ./xohtml/xohtml.sh -b $PWD/../xohtml -c './tests/core/test_01.test --libxo:HPx' > /tmp/foo.html > > -b gives the base directory to find css and js files > -c gives the command to run > > These options will likely change before it's finished. > > I've opened issues for all these on github. I'm afraid there might have mentioned a few more (minor) issues in this mail ;-) Thanks again for this valuable addition to FreeBSD. I've been wondering whether it might be a good idea to create a "wish list" on a Wiki page, where utilities that might benefit from libxo could be listed (for example fstat, pstat, ps, ls, ifconfig, ...). Adding libxo support could be a good Junior Hackers task and might even attract a few people to work on FreeBSD ... Best regards, STefan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?548313E1.1040701>