> > * Throughout: there are a number of places where "?" is currently > > returned, which basically indicates a failure (e.g., 489). > > Shouldn't these cases now return some failure to the output engine, > > which then elects to display this information however it sees fit? > > I don't know, do we want to display "?" or nothing at all? > Returning an error would display nothing at all. Also dladm(1m) says > > (for show-linkprop) > The current (or persistent) property value. If > the value is not set, it is shown as --. If it > is unknown, the value is shown as ?. Persistent > values that are not set or have been reset will > be shown as -- and will use the system DEFAULT > value (if any). > I'm not sure if ipmpstat makes such distinctions.
It does. Specifically, it uses "unknown" to mean "the value could not be retrieved"; there is a separate possible definition of "unknown" which means "the value was retrieved, but actually had the value "unknown" which it will report as "unknown"). I view the use of "?" as a way to represent an unknown value (such as using "--" to represent an empty value in non-parsable mode) as an output policy choice which should be handled by the output engine you've built, not hardcoded into ipmpstat and others. I will file a bug to have "?" documented in ipmpstat(1M). > > * 47: There should only be a "libfoo.h" when there is a corresponding > > libfoo.so. This header file should be named something more > > appropriate like "ofmt.h". Likewise, fix comment on line 77 > > I was following the precedent set by libdladm/common/lib* (many files > of which were added by Clearview :-) The libdladm header collection was a bit of a compromise and in retrospect a confusing one -- but at least everything beings with "libdl". > > * 90: Why isn't `ofmt' static? Also, existing convention in this > > file would have named this `oh' or `ofmth', not `ofmt'. > > It can be static. "oh" sounded a little startled :-), but sure. Yeah it's a little silly ;-) > > * 315: Unclear why OFMT_NOMEM is needed; isn't that what a NULL return > > from ofmt_open() means? > > OFMT_NOMEM could also be encountered if we ran out of memory at > line 174 of libprintutil.c, and we don't return a NULL handle > for that case.. Why not? > > * 1295: Formatting here is now odd. Suggest adding a tabstop after > > "0," on all the lines so that everything aligns again. > > yes, I noticed this too. and I can add the tabstop, though group_fields will > stick out longer than the other *fields[] rows. If you pad things out per my recommendation above, I think everything should line up. -- meem
