Hi, Michael Thanks for review!
Michael Hunter wrote: > On Wed, 09 Jan 2008 13:48:36 -0800 > Zhenghui.Xie at Sun.COM wrote: > > >> http://cr.opensolaris.org/~xzh/walkprop/ >> >> This webrev includes >> 1. the property-walk-through command >> 2. a fix for the bug Jia pointed out >> 3. a better/user-friendly display format (for instance, it will accept >> and report "true/false" for a boolean value parameter). >> >> Thanks for review! >> >> BTW, in case anybody curious, the up-to-date nwamcfg(1M) man page is on >> the way... >> > > nwamcfg.c:313,335 what is this suppose to be? maybe > sizeof(ncu_prop_table)/sizeof(ncu_prop_table[0]) > way nit: personally I like SIZE or NELEMS for number of elements in a > table and LEN for length of buffers because I think they carry more of > the meaning then NUM > > I think what you mentioned is a valuable suggestion. The reason I used this format is to keep consistent with the format in libnwam_ncp.c line134 and libnwam_enm.c line381, where both of them have the similar table and used this format to calculate the table size. > nwamcfg.c:826 boolean_t > > nwamcfg.c:894-6 nit: Above (896,8,...) you enclose single line blocks. > Here in an else with a single line unbraced block and a braced else > block. I think you should make these consistent. Personal choice is > to always use blocks unless there is some if/else if/.../else block all > with single line bodies but consistency is the more important goal. > > nwamcfg.c:910-915 way nit: All of the inner structure initializers have > the extra ',' at the end. That looks a little wierd. The one after the > last array element on 915 makes some sense and is specially allowed in > the ANSI c specification due to automatic code generators FWIR. In our > less mechanical code writing duties it means you can add elements to > the array without changing the last line thus limiting the size of your > diff. Yea, seems like a lot of thought for pretty modest returns. > > nwamcfg.c:1377 wonder if you should have a define for whatever an > inband bad nwam_data_type_t is. > > nwamcfg.c:1404 const char *input_str? > > nwamcfg.c:1537,1573 nit like 894 above > > nwamcfg.c:2275,2314 The input is not guaranteed to be newline termianted. > > all accepted. > libnwam_ncp.c:1122 you might assert that i doesn't get > prop_num > I probably didn't get what you mean. Would you please elaborate? Thanks! -Jan
