> > * General: The code seems to no longer print a helpful message
> > when "-o all" is used in parsable output mode.
>
> that should be the application's choice, right?
That won't work. For parsable output mode, the consumer always needs to
ask for exactly what it wants, or it has no way to interpret the output
(consider that the output of "all" will change over time).
> > * 35: <macros.h> is ancient and AFAIK unmaintained. Recommend
> > using <sys/sysmacros.h> instead.
>
> I pulled macros.h to get min(). sysmacros.h does not have min().
> What's the right header file for this?
<sys/sysmacros.h> has MIN().
> > * 476-478: Again, OFMT_NOMEM should go away.
> As I understood it, we agreed that ofmt would be null in every
> case where ofmt_open ran out of memory (even in the middle of parsing
> the fields_str etc.). But I can't return OFMT_SUCCESS and a null handle-
> that would be misleading! So I have to return some error code, to let
> the caller know that things didn't work out.
I see.
>
> > * 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.
> Either way, you'd have to check if the sp->s_fields[i] found a match
> in the template (i.e., you'd have to check if j == nfields). And,
> unless you over-allocate the os_badfieldsp[] (which I don't think
> is a good idea- too much planning and malloc around the less
> frequently expected case of failure), you'd still have to muck
> with mallocs on the fly. So I'd prefer to leave the code as it is.
This is wildly overcomplicated as-is. Assume that you allocate an
os_badfields[] array when you create the ofmt_state_t, and that
ofmt_close() also frees this array. Then the entire logic would be:
if (j == nfields) {
if ((badname = strdup(template[j].of_name)) == NULL)
goto nomem;
os->os_badfields[os->os_nbad++] = badname;
continue;
}
Then in ofmt_strerror(), you'd do:
if (os->os_nbad == 1)
(void) strlcpy(buf, "unknown output field", bufsize);
else
(void) strlcpy(buf, "unknown output fields", bufsize);
for (i = 0; i < os->os_nbad; i++) {
(void) strlcat(buf, " `", bufsize);
(void) strlcat(buf, os->os_badfields[i], bufsize);
(void) strlcat(buf, "'", bufsize);
}
> > * 198: What if strdup() fails?
> accept that I have to bail out: see discussion on OFMT_NOMEM.
OK, please add a new `nomem' label to ofmt_open() to consolidate the
duplicate error paths for this case.
> > * 402: ... and if the caller returns neither of these values?
> That would be an error, and output is skipped. What else would you
> suggest we should do in this case?
We could assert() that the caller does not pass back another value.
But if we think we'll only ever have two values, we might be better
off just having the caller return B_TRUE or B_FALSE depending on whether
they were able to successfully return a value.
> > * 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.
> Ok.
Now it prints "Success", which will look a bit silly since it will not be
at the start of a sentence. (Same with some of the other messages.)
(I know libc does this; it's always annoyed me.)
> > * 470: Is it really "ignored" in the case of parsable output
> > mode?
> No. "ignored" will only be appended in !parsable mode in the modified
> webrev.
I see the new code tries to add "ignored" conditionally. Again, this is
not localizable since not all languages allow "ignored" to be added to a
sentence like in English. You need a distinct error message, like
ipmpstat originally had.
Some other comments:
* 69, 73, 79: Seems like we could just remove `maxlen' from this
implementation; its unclear what it's doing for us aside from
complicating the code.
* 203-204: Unclear why we try to get clever allocating os_fields[]
as part of the initial `os' allocation. Why not just do a
separate calloc()?
* 384-385: This should fit on one line.
* 408-423: Code still erroneously converts to upper-case and will
dump core if strdup() fails. This should be removed as per my
earlier comments.
* 472-474: No need to duplicate this case; just handle it at 446.
--
meem
