> I have generalized and consolidated the duplicated code in dladm,
> flowadm, ipmpstat for printing output as part of
>
> 6782154 one copy of parse_output_fields() & friends is enough.
>
> The code is available for review at
> http://cr.opensolaris.org/~sowmini/parsegoop/
For starters, here's my input from an initial run through ipmpstat.c.
Once we converge here, I'll go through the rest.
* 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?
* 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
* 90: Why isn't `ofmt' static? Also, existing convention in this
file would have named this `oh' or `ofmth', not `ofmt'.
* 90-102: All of these declarations and definitions belong properly
sorted into the list at 183-194.
* 209, 213: These two declarations should be neighboring, and most
logically after the `ofields' declaration. Also, `ofmterr' would
better match the conventions in this file.
* 302: Would prefer this to be declared at the top. More importantly,
there should be an actual #define for the size, provided by ofmt.h.
* 312: Stray space after "memory", but moreover, I would prefer to use
the standard libc strerror() logic here.
* 315: Unclear why OFMT_NOMEM is needed; isn't that what a NULL return
from ofmt_open() means? (And ofmt_create() seems a more fitting
name in this context than ofmt_open()).
* 301-327: More generally, this code is just ugly. Suggest:
if (ofmterr != OFMT_SUCCESS) {
if (ofmt == NULL) {
errno = ENOMEM;
die("cannot create output handle");
}
/*
* If some fields are bad and we're in human-friendly output
* mode, print a warning. Otherwise, die.
*/
(void) ofmt_strerror(ofmterr, ofmt, errbuf, sizeof (errbuf));
if (status == OFMT_BADFIELDS && !(opt & IPMPSTAT_OPT_PARSABLE))
warn("%s\n", errbuf);
else
die("%s\n" errbuf);
}
* 605: What is the point of `ret' here?
* 753: This is a partial failure. I think we should probably return
OFMT_SUCCESS here so that at least some of the information can be
retrieved (the ordering of logic in this function is careful fill
in all of the fields that do not require IPMP group info first.
* 1128: Comment is stale (references to `ih'), no longer properly
formatted, and ends in a semicolon.
* 1295: Formatting here is now odd. Suggest adding a tabstop after
"0," on all the lines so that everything aligns again.
--
meem
