> http://zhadum.east.sun.com/export/ws/sowmini/brussels/parsegoop2cv.next/
[ Hopefully this webrev isn't too old; I see some stuff that I thought
you'd already agreed to change. ]
ofmt.c:
* General: i_() prefix seems unnecessary and also used erratically
(e.g., the split() functions are internal but aren't prefixed).
I'd recommend just removing the i_() stuff as it doesn't really
make things any clearer in this case.
* General: Ordering of the functions seem strange -- e.g.,
ofmt_close() is defined before ofmt_open(), and the split()
functions bisect a bunch of ofmt() functions.
* General: The code seems to no longer print a helpful message
when "-o all" is used in parsable output mode.
* 34: What does this file use from <stropts.h>?
* 35: <macros.h> is ancient and AFAIK unmaintained. Recommend
using <sys/sysmacros.h> instead.
* 43: s/pointer/pointers/
* 52: What is ps_fields[i]? In general, this comment seems to
refer to things that are unrelated to this file.
* 63: Why isn't `os_err_data' `char *'? Also, os_errdata seems
more consistent with other fields in this structure.
* 69-80: Seems like this function should just be inlined into
ofmt_update_winsize().
* 94: We only use `of' in one place (line 96); seems clearer to
just inline it.
* 97: No need to check for NULL here.
* 106: ofmt_ prefix on ofmt_flags' seems needless.
* 118, 120: Needless braces.
* 126, elsewhere: why is OFMT_MAX_FIELD_LEN needed? I can't
recall why we limited the field name length in dladm, but having
a library imposing arbitrary limits like this seems just wrong.
* 134, 139, 176: I thought we agreed to remove OFMT_NOMEM?
* 158-196: This institutes more policy here than I would've
expected. In particular, I would've thought we'd just maintain
a parallel array of bad fields (e.g., an os_badfields[] member
in ofmt_state_t) and then decide what to do with them later.
This should also make the code a lot simpler -- e.g., you can
just worst-case size it to `nfields' pointers and strdup()
entries into it. You can then return OFMT_BADFIELDS if
os_badfields[0]->of_name != NULL.
* 181: Unclear why we test !first_bad_field rather than
first_bad_field.
* 184: bzero() seems excessive; os_err_data[0] = '\0' will do.
* 187-191: It'd be preferable to collect all of the field names
even when in parsable mode so that a single diagnostic can
provide all the info rather than forcing the script writer to
fix the problems one at a time.
* 198: What if strdup() fails?
* 332: Unclear why this is declared local to this branch when
several of the other variables declared at 322-325 are only
used by one of the two `if' branch. The code is simple enough
that it'd read better to hoist this declaration to line 322.
* 348: STR_UNDEF_VAL should probably be OFMT_VAL_UNDEF or
somesuch.
* 373: Why `statep' here rather than `os'? Inconsistent and leads
to line wraps like 383-384.
* 381: `parsable' variable seems unnecessary; just use os_parsable.
* 393: `fn' seems unnecessary and confusing.
* 394: Again, bzero() seems excessive.
* 401: STR_UNKNOWN_VAL should probably be OFMT_VAL_UNKNOWN or
somesuch.
* 402: ... and if the caller returns neither of these values?
* 427: There's really no need for this strdup() since the buffer
is already owned by ofmt() (and what happens if strdup() fails?)
That said, I think this policy decision should be up to the
caller, which should call ofmt_open() with the fields written
in the manner it would like them to be displayed.
* 465: Printing "success" seems preferable.
* 470: Is it really "ignored" in the case of parsable output
mode?
* 471: This will defeat localization. You need to define two
separate strings, one for the singular case and another for
the plural case.
* 471: Is the cast of `os_err_data' really needed?
* 474: "no valid output fields specified" seems clearer.
* 476-478: Again, OFMT_NOMEM should go away.
* 480: "ofmt" should be included in this error message to make
it easier for the field to map the message back to a cause.
--
meem
