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. > <sys/sysmacros.h> has MIN(). fixed. > > 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. 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 :-) > OK, please add a new `nomem' label to ofmt_open() to consolidate the > duplicate error paths for this case. done. > > > * 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 "?" ?) > 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.) fixed. > 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. 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. > * 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. > * 384-385: This should fit on one line. Ok. > * 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. Oops. yes, removed. > * 472-474: No need to duplicate this case; just handle it at 446. ok. --Sowmini
