Thanks for the review! > > > http://zhadum.east.sun.com/export/ws/sowmini/brussels/parsegoop2cv.next/webrev
has been updated, and I also have > > > http://zhadum.east.sun.com/export/ws/sowmini/brussels/parsegoop2cv.next/webrev.meem with just the deltas for this review. > [ Hopefully this webrev isn't too old; I see some stuff that I thought > you'd already agreed to change. ] It should be.. if I left something out and it is not covered in the list below, it was unintentional, so please flag it.. > ofmt.c: > > * General: i_() prefix seems unnecessary and also used erratically > (e.g., the split() functions are internal but aren't prefixed). > I'd recommend just removing the i_() stuff as it doesn't really > make things any clearer in this case. Ok > * General: Ordering of the functions seem strange -- e.g., > ofmt_close() is defined before ofmt_open(), and the split() > functions bisect a bunch of ofmt() functions. I've tried to reorder it more consistently.. > * 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? e.g., in ipmpstat.c: 267 if (opt & IPMPSTAT_OPT_PARSABLE) { : 271 } else if (strcasecmp(ofields, "all") == 0) { 272 die("\"all\" not allowed in parsable output mode \n"); 273 } etc.? > * 34: What does this file use from <stropts.h>? didn't need it, cleaned up. > * 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? > * 43: s/pointer/pointers/ Ok. > * 52: What is ps_fields[i]? In general, this comment seems to > refer to things that are unrelated to this file. should be os_fields. fixed. > * 63: Why isn't `os_err_data' `char *'? Also, os_errdata seems > more consistent with other fields in this structure. > * 471: Is the cast of `os_err_data' really needed? it can be a char*. Ok to rename. > * 69-80: Seems like this function should just be inlined into > ofmt_update_winsize(). Ok. > * 94: We only use `of' in one place (line 96); seems clearer to > just inline it. Ok. > * 97: No need to check for NULL here. Ok. > * 106: ofmt_ prefix on ofmt_flags' seems needless. Ok. > * 118, 120: Needless braces. Ok (but modified by changes due to next comment): > * 126, elsewhere: why is OFMT_MAX_FIELD_LEN needed? I can't > recall why we limited the field name length in dladm, but having > a library imposing arbitrary limits like this seems just wrong. I don't see, either, why the field-name is bounded in dladm. But I use the max field len to construct the string for "-o" when ofmt_str is called with NULL (not strictly necessary, but I do it for correctness and consistency). Even in this case, the max_field_len does not have to be bounded - I can just compute it on the fly. > * 134, 139, 176: I thought we agreed to remove OFMT_NOMEM? > * 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. > * 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. > * 181: Unclear why we test !first_bad_field rather than > first_bad_field. I can go either way. fixed. > * 184: bzero() seems excessive; os_err_data[0] = '\0' will do. Ok. > * 187-191: It'd be preferable to collect all of the field names > even when in parsable mode so that a single diagnostic can > provide all the info rather than forcing the script writer to > fix the problems one at a time. Ok. I just felt it was needless parsing, given that we have agreed that any bad field in parsable mode is fatal. > * 198: What if strdup() fails? accept that I have to bail out: see discussion on OFMT_NOMEM. > * 332: Unclear why this is declared local to this branch when > several of the other variables declared at 322-325 are only > used by one of the two `if' branch. The code is simple enough > that it'd read better to hoist this declaration to line 322. Ok. > * 348: STR_UNDEF_VAL should probably be OFMT_VAL_UNDEF or > somesuch. Ok. > * 373: Why `statep' here rather than `os'? Inconsistent and leads > to line wraps like 383-384. Ok. > * 381: `parsable' variable seems unnecessary; just use os_parsable. Ok. > * 393: `fn' seems unnecessary and confusing. Ok. > * 394: Again, bzero() seems excessive. Ok. > * 401: STR_UNKNOWN_VAL should probably be OFMT_VAL_UNKNOWN or > somesuch. Ok. > * 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? > * 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. > * 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. > * 471: This will defeat localization. You need to define two > separate strings, one for the singular case and another for > the plural case. Ok. > * 474: "no valid output fields specified" seems clearer. Ok. > * 480: "ofmt" should be included in this error message to make > it easier for the field to map the message back to a cause. Ok. --Sowmini
