On (02/09/09 14:14), Peter Memishian wrote:
> 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?
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.
> * 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 :-)
> * 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.
> * 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.
Ok, I'll fix the above.
>
> * 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..
> (And ofmt_create() seems a more fitting
> name in this context than ofmt_open()).
We use the "open/close" syntax in other libraries (e.g., dladm_open,
dlpi_open, fopen etc.). So I think we should stick with
that convention.
> * 301-327: More generally, this code is just ugly. Suggest:
thanks. ok.
> 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?
not needed, can be removed.
>
> * 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.
Ok.
> * 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.
I'll send out an updated webrev shortly.
--Sowmini
