On (02/24/09 11:37), Peter Memishian wrote: > > and return OFMT_NOFIELDS if null str or str == "all" is specified > > in the os_parsable case. > > 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. > OK, but your new code doesn't handle strdup() failure or malloc() failure, > and has a needless `ptr' declaration. True. fixed. > Also, it seems more logical to test > if os_badfields is NULL since the code assigns os_badfields. ?? where? in ofmt_close? > > BTW, the code you had suggested does not print "ignored" for the !parsable > > case. Rather than complicate this with 4 separate strings for > > field/fields/ignored/<not ignored>, I've compromised field/fields > > and condensed into "field(s)". I think the reader will understand > > and tolerate :-) > > 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(). And the the translation dilemma has evidently been solved for dladm. :-) But I have unrolled that logic to humor you. > If the value cannot be retrieved due to an unexpected failure, the caller > should return B_FALSE and the ofmt engine should display "?". My question > is whether there are other cases. It seems like there are three cases: > > * Property is supported on a given link, and has a value: display > that value (with minor fix-ups in non-parsable mode, like --). > > * Property is supported on a given link, but the value cannot be > retrieved: display "?" > > * Property is unsupported on a given link: higher-level > functionality ensures we don't get here -- e.g., dladm show-link > won't list properties that are not supported on a given link, > and will fail with an explicit error message if the admin tries > to display an unsupported link property. Given that we already make fine distinctions between "UNKNOWN" and "UNDEFINED" today, I think there could be more than 3 cases in the future. 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). > > > * 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. > > I see you updated the code, but the comment on line 70 still mentions > `maxlen'. fixed. > More comments on the updated code: > > * 30: Now that toupper() is gone, no need for <ctype.h>. > > * 45-46: No need for these static declarations. > > * 51: This comment still seems a bit presumptuous; who's to say > the ofmt_field_t array is global or that use is tied to a > subcommand? > > * 167: Needless initializer. > > * 200, 202, 222, 234, 359, 361, 363: Remove needless braces. Ok to all of the above. > > * 226: Why `sizeof (char **)'? I would've expected `sizeof (char *)'. > > * 242: Presumably this comment means to reference `str'? There's > no fields_str in scope. yes. > * 379, 380: Could initialize these at 376-377. > > * 383-385: This comment seems a tad obvious :-) yes, fixed. See webrev.meem in the usual place. --Sowmini
