Webrev is at: http://zhadum.east/export/ws/am223141/checkout-area/nwam1-fixes/webrev/
Most of the comments have been accepted and won't be talked about here. (All code-review comments are at http://wikis.sun.com/display/NWAM/Phase+1+Code+Review+Feedback). The original webrev is at http://cr.opensolaris.org/~mph/nwam1_cr/. > meem-109 nwamcfg.c: many of the same comments I made for nwamadm apply > to this file ? e.g., zerr(), die_nwamerr(), CMD_* vs command tables, > etc. In the interest of brevity, I have not repeated previous comments > that apply directly to nwamcfg (e.g., problematic use of basename()). Rather than die_*() commands, the following commands exist. Can't use die_*() as we don't want to exit because of interactive-mode. * zerr() has been renamed to nerr(). * nwamerr() takes in nwam_error_t and prints the string representations. I've also added nwamcfg.xcl and XGETFLAGS in the Makefile. > meem-110 nwamcfg.c: a lot of very repetitious code ? I have enumerated > a number of instances below. FWIW, a lot of these issues are due to > the libnwam API itself ? e.g., sets of parallel functions that differ > only in the handle they take and their name. It seems like an OO > design would've simplified the code considerably ? e.g., each object > could've had an ops vector associated with it that the various callers > could use without repeatedly concerning themselves with the type of > the object. I have taken your suggestion on "get_handle_type()". I called it "active_object_type()" and returns NWAM_OBJECT_TYPE_* depending on which handle is not NULL. I've added functions that taken in the object type and call the respective library functions. get_func(), set_func(), clear_func(), walkprop_func() have been considerably shortened thanks to your approach. The object type is consulted when library function calls are to be made. At other times, the helper functions are passed the object type. Also, code that duplicated logic of commit_func(), destroy_func(), end_func() has been factored into do_commit() and do_cancel(). > meem-118 nwamcfg.c'163, 167, 187, 189, 193, 214: What is the > significance of these comments? To show where the constants come from. > meem-120 nwamcfg.c'240, 286, 304, 359: Why are these all different > structures? They all have identical contents and field names. These > should be folded into a common nwamcfg_prop_table_entry structure and > all members should have a consistent prefix for easy cscoping. These structures have been modified to map the PT_* constants (used by the parser) to the property names. The other fields in the table has been removed. Library calls are used to determine the type of the value and whether it is multi-valued? > meem-126 nwamcfg.c'1043: prompt[] only accounts for MAX_NWAM_TYPE_LEN > once, but there can be two types. fixed this. found that the object name can also appear twice (in the case of NCUs). > meem-130 nwamcfg.c'1390-1392: Under what situations do callers pass in > NULL? export_func() and list_func() can pass in NULL strings. Checking and returning NULL here clears up a lot of duplicate code in the respective functions. > meem-138 nwamcfg.c'1752-1764: What happens to nwamcfg when this to > fails part-way through? Whatever is destroyed in gone. Once an error is hit, the remaining objects are not destroyed. Returns to the nwamcfg> prompt if in interactive-mode. > meem-150 nwamcfg.c'2389-2399: Doesn't strtol() already provide this > ability by checking if isdigit(*endptr)? Letters are returned as 0 and thus the need to check if all values are digits or not. > meem-153 nwamcfg.c'2543-2544: How will this value be maintained? Seems > like nothing uses more than two values though. maintained in nwamcfg. Not likely that the number of values will increase in the future. Made the constant slightly bigger so that there's no need to change later. > meem-154 nwamcfg.c'2777-2780: `return (!prop_found)' seems simpler (if > the concern is clarity, giving the boolean another name (like > `show_prop') would help. Keeping this variable as prop_found. That's because if the code reaches after the for-loop, then it means that the property was found in the table but did not match any rule, thus it should not be shown. show_prop is not the intended meaning at this point in the code. > meem-158 nwamcfg.c'3181-3258: This is really hard to read. One minor > improvement would be to have the logic at 3193-3257 just build the > string arrays, and have a single loop iterate through them at the end > and centralize the fprintf() and NWAM_VALUE_DELIMITER_STR logic. generating the string would be the same with snprintf() replacing fprintf(). > meem-159 nwamcfg.c'3322-3334: Needs to share a common implementation > with output_propname(). The reason this is different is because the location properties have longer names than the other objects. If the other objects used the same tabbing as location, the values would appear spaced out more. The reason for the difference is just making output prettier. Using "%-25s" would print: ENM:foo activation-mode manual enabled false stop "/bar" start "/bar" and "%-16s" will print ENM:foo activation-mode manual enabled false stop "/bar" start "/bar" > meem-177 nwamcfg.c'4899-4908: What would go wrong if we attempted to > use a file that wasn't a "regular file"? checked with S_ISREG flag a few lines later. Thanks, Anurag
