> http://zhadum.east/export/ws/sowmini/brussels/parsegoop2cv.next/webrev.meem
>
> for the changes discussed here. (main webrev in parsegoop2cv.next
> has also been updated)
>
> On (02/23/09 22:23), Peter Memishian wrote:
> > > > * 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).
>
> Ok, I've modified to have ofmt_open check against this
> 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.
> > 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:
>
> Ok, but since I'm uneasy with unconditionally allocating the badfields[]
> and carrying this around in the handle for the error-free case,
> I'm only allocating the os_badfields if at least 1 bad field is found.
OK, but your new code doesn't handle strdup() failure or malloc() failure,
and has a needless `ptr' declaration. Also, it seems more logical to test
if os_badfields is NULL since the code assigns os_badfields.
> 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.
> > > > * 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.
>
> but the callback function *can* encounter an error (e.g., some error
> contacting dlmgmtd, or something in an aggr changed and the system
> call failed). One solution is to always print "--" in this case.
> (Or maybe "?" ?)
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.
> > * 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.
>
> Well, it's used to construct the actual fields str when NULL has been
> specified.
> I figured we may as well be consistent in constructing the split_t, but
> I can just as easily rip out this code.
I see you updated the code, but the comment on line 70 still mentions `maxlen'.
> > * 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()?
>
> You are always going to need the os_fields[] for an os. You could do
> the callocs and separately check that each one succeeded (and free all
> memory that's already been allocated when the later one fails), or you
> could just do the whole thing as one contiguous chunk. I don't think
> there's a significant advanatage/disadvantage to either, so I chose
> the latter.
The disadvantage is that the code is harder to read and looks like it's
doing something subtle, especially with the dance at line 204. By the
time you add the comment to explain it, you haven't saved any code.
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.
* 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.
* 379, 380: Could initialize these at 376-377.
* 383-385: This comment seems a tad obvious :-)
--
meem
