Date: Tue, 9 Dec 2014 18:45:16 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Stefan Esser <se@freebsd.org> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@freebsd.org>, Phil Shafer <phil@juniper.net> Subject: Re: [Patch] updated: Add JSON and XML output to pciconf (libxo support - D1206) Message-ID: <20141209164516.GB97072@kib.kiev.ua> In-Reply-To: <548723D0.1000402@freebsd.org> References: <201412082114.sB8LEJnR056654@idle.juniper.net> <548723D0.1000402@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Dec 09, 2014 at 05:31:12PM +0100, Stefan Esser wrote: > 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. If it is required that the name be identical, it makes more sense to require some handle to be passed to functions instead of the string. I.e. either there should be some atom-like calls which convert string to identifier, or first call should return handle passed to consequent calls. This is obviously a note WRT libxo interface, not your patch. > > 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). .... > OTOH, I'd love to see the output generated on an AMD64 system > with HyperThreading information. There are no AMD processors which support HyperThreading. I suspect you mean HyperTransport there. Any relatively modern AMD CPU, in particular, all chips capable of running amd64, use hypertransport for northbridge. Look at netperf cluster for available resources, there are several AMD machines there https://wiki.freebsd.org/TestClusterOneReservations
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141209164516.GB97072>