Date: Tue, 09 Dec 2014 17:31:12 +0100 From: Stefan Esser <se@freebsd.org> To: Phil Shafer <phil@juniper.net> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org> Subject: Re: [Patch] updated: Add JSON and XML output to pciconf (libxo support - D1206) Message-ID: <548723D0.1000402@freebsd.org> In-Reply-To: <201412082114.sB8LEJnR056654@idle.juniper.net> References: <201412082114.sB8LEJnR056654@idle.juniper.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Am 08.12.2014 um 22:14 schrieb Phil Shafer: > Stefan Esser writes: >> D1206 on reviews.freebsd.org (https://reviews.freebsd.org/D1206). > > Patch looks good. Some nits: Hi Phil, thanks again for your effort to help me understand libxo! > The names for xo_open_list and xo_open_instance need to be > identical, so: > >> @@ -554,9 +698,14 @@ >> /* Walk the capability list. */ >> express = 0; >> ptr = read_config(fd, &p->pc_sel, ptr, 1); >> + xo_open_list("capabilities"); > > should be: > > xo_open_list("capability"); > > to match: > >> + xo_open_instance("capability"); I just re-read the man-page for xo_list_open() and it clearly says: "The name given to all calls must be identical, and it is strong suggested that the name be singular, not plural, [...]", which I had wrongly understood to only relate to opening and closing of elements. I had noticed, that the parameter of xo_open_list() is printed as the array name in JSON, while the parameter of xo_open_instance() is used as the element name in XML - it seemed more natural to use the plural for the array name ... (and I had not noticed the sentence I quoted above). > Similar for: > >> + xo_open_list("extended-capabilities"); >> ... >> + xo_open_list("error-categories"); >> ... >> + xo_open_list("base-addresses"); Yes, there were even more, I have fixed them in my local version (not yet uploaded to the reviews site). > If you turn on the warn flag, you should get these issues reported > (--libox=warn). This did not work for me - --libxo=warn generates no output at all for these examples. >> + xo_emit("{:name/unknown}"); > > Should "name" be "capibility-name", to be consistent? Fixed, thanks. > Add some point the printf calls need to be removed. Sure, I just wanted to keep them around in the sources until those capabilities that I do not know and cannot test have received a review (especially Hyper threading specific information). I'm afraid that there will be requests to change quite a number of field labels, once this patch is committed to -CURRENT and I really hope for other interested parties to > It would be great to see some example xml, json, and html output for this > command, pretty printed of course. I have uploaded sample output (with the link/instance names fixed according to your review): https://people.freebsd.org/~se/pciconf-lbcevV.tar.bz2 I'm afraid that the verboseness is quite high and that some fields are poorly named. OTOH, I'd love to see the output generated on an AMD64 system with HyperThreading information. (Offätopic: is it possible to attach such information to the review on Phabricator? I noticed that due to the markup language used, it is hard to put structured information into comments. Marking as text that should not be interpreted as markup did not work when I tried it ...). BTW, a few remarks and questions regarding libxo: It seems, that libxo/xo.h depends on stdio.h (or at least stdarg.h) for functions that use FILE or varargs. This is not mentioned in the respective man-pages for the affected functions (and I think it should be ...). Is there a description of the rules, which decide whether quotes are put around JSON formatted information elements. It seems, for example, that use of %d in format strings suppresses quotes, which was unexpected, if the format is e.g. "xyz%d" (which is a string, despite the %d ...). And formatted output that contains blanks within the field is probably also a string (even "%d %d", hmmm, but "%d.%d" might be a number ...). But I have to admit, that I do not really know the rules for quotes around data fields in JSON. (E.g. must I write "true" to represent a string value of "true", or could I also use true without quotes to represent a truth value? What do parsers do if there are unquoted words or words separated by blanks?) Are there any plans for a versioning scheme for XO extended programs. E.g. if I need to change some of the elements or labels in pciconf, how do I include the information about the file schema being the old or the new one in an output file. I could f.i. just add an xo_emit(":e/schema-version/1") at the head of the file, to make the file format version explicit in the generated output ... (Or rather: is there a concept how to let a parser know which XML schema to expect when looking at some input file, if the schema has been changed over time?) Thank you for the time to review my attempt to grok libxo! When pciconf is ready, I'll have a look at other tools that could be improved by adding XO support ... (and I hope I'll be fluent in XO and will not require as much support ;-) ) Best regards, STefan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?548723D0.1000402>