> > That will produce a misleading error message. We need a new ofmt error
> > code for this case.
>
> Ok. I was trying to avoid the too_many_error_codes solution. But I
> have OFMT_PARSE_NOFIELDS. And no, I don't like that const name either.
> So if you have better thoughts, feel free to suggest.
In this case, I think having more error codes is actually good because it
will result in clearer error messages -- and furthermore, we only need to
write them once :-) Another missing error case: parsable mode must require
a field list (for the same reason). I doubt the name of these matter too
much since ofmt_strerror() will likely be the lone consumer, but I'd go
with OFMT_EPARSEALL and OFMT_EPARSENONE. As a related minor namespace
nit, it'd be preferable to start all the ofmt errors (except OFMT_SUCCESS,
which isn't really an error) with OFMT_E to make it clear it's an error
code. The libipmp and libdlpi libraries follow the same convention.
> > Also, it seems more logical to test
> > if os_badfields is NULL since the code assigns os_badfields.
>
> ?? where? in ofmt_close?
In ofmt_open(), I'd find this more logical:
err = OFMT_BADFIELDS;
if (os->os_badfields == NULL)
os->os_badfields = malloc(...
> > So we're going back to 1980's error messages because we don't want to
> > enumerate the four possibilities? :-( Also, while "field(s)" may not be
> > too ugly in English, other languages may have completely different forms
> > for singular and plural, so the localized version will be problematic.
>
> Not 1980's. Only as far back as dladm which uses this in do_show_link().
I'm thinking of the classic MS-DOS summary for the dir command:
C: >dir
GROUPS 65535 10-01-87 4:14p
MEETINGS TXT 267 11-11-87 11:10a
MEMBERS <DIR> 12-29-87 3:29p
--> 3 file(s) 65792 bytes
2114816 bytes free
> Given that we already make fine distinctions between "UNKNOWN" and
> "UNDEFINED" today, I think there could be more than 3 cases in the future.
What is "undefined"? AFAIK, a property's value is always defined, though
it may have an empty value.
> So I vote that we make this an assert (i.e., value returned from
> callback must not be anything other than success/unknown) so that
> expand the mnemonics later, if needed (I've not changed this part of
> the code in the webrev yet.. want to get agreement on this first).
Hmm -- it seems like the current return value of "unknown" actually means
"failure". (If not, please clarify in what case it has another meaning.)
If so, "success" and "failure" pretty much run the gamut of possibilities.
Is the intent to provide more fine-grained notions of failure in the
future? In any case, I don't feel too strongly about this since it
doesn't add much complexity into the API. I'm mostly curious what you
had in mind.
> > * 45-46: No need for these static declarations.
Oops, I meant to remove this comment as I subsequently re-read the comment
on line 36 that makes it clear that you're trying to provide the complete
mini-API for this bit of functionality. Sorry about that.
--
meem
