Hi Anand, I haven't built yet, so I apologize if I'm way off base. These comments are based on just skimming the code (and prior discussions with you and others).
1) It seems that both "Password20" and "Password" would be output on a bmc-config --checkout. Only one should be output. 2) In functions like bit_rate_string & bit_rate_number in bmc-serial- conf-section.c, you hardcode values straight out of the spec. case 6: return "9600"; When possible you should use the defined values from libfreeipmi, to keep things consistent and remove the possibility of bug typoes. case IPMI_BIT_RATE_9600_BPS: return "9600"; 3) in k_r_checkout() char k_r[21]; Similarly, lets use the correct #defines (IPMI_MAX_K_R_LENGTH in this case) for array lengths. This extends to other places too. 4) Also in k_r_checkout() k_r[21] = 0; The largest index is 20, so this corrupts memory. Similarly elsewhere. 5) In functions like bit_rate_string, you return "" if the value stored in the bmc doesn't match any known value. Could it be "unknown" instead? I feel that something like "Unknown" will give the user a better idea that something is incorrectly set. Another reason to do "Unknown", if the user doesn't set the value, bmc- config will display invalid input when the user attempts to --commit, forcing the user to submit a legit value. Otherwise, won't a --commit simply ignore the blank? 5) I notice throughout almost all the code that you don't check the return value of strdup() for == NULL. 6) I notice that you pass a base of '0' for most calls to strtol. According to my manpage, '0' means a base of 16. Although some config might take hex as input, I'm going to assume that most actually want to take decimal/base 10 as input. Is it possible most of your code just simply works b/c we never input anything higher than 9? 7) I notice that many times you do: num = strtol (value, &endptr, 0); if (*endptr) return 1; after a call to strtol(). I think this is a typo. You want to return -1? 8) I also noticed that in character_accumulate_interval_validate() you didn't check the range of the actual value. 9) Due to the increasing number of key-value pairs that are machine dependent, I had A.B. have bmc-config output "unknown key foobar" on a bmc checkout only when a -v verbose option was selected by the user. It doesn't seem to be implemented. (See bug #16113) Instead, you seem to be using the verbose option for debugging instead? (based on code I see in bmc-parser.c) How about a --debug option for debugging instead. And have debugging only built if the user builds with --enable-debug during configure. See my code in ipmipower for examples. These options also need to be documented in the manpage. 10) One nit-picky thing. You use the if () { foo } else { bar } indenting/bracket style for coding. I have nothing against this, but it currently differs from the rest of FreeIPMI. Perhaps we want to stay consistent throughout FreeIPMI? BTW, I noticed you seem to checkout a config based on the number of users on the given channel. Cool! Al On Wed, 2006-07-05 at 11:30 -0700, Anand Avati wrote: > Hi, > I have attached the files for adding bmc-config in C. > > bmc-others.diff <-- apply this patch > bmc-config.tar.bz2 <-- extract this in the top level dir of freeipmi > > you also may want to delete bmc-config related files in scheme since > make install might conflict each other. > > regards, > avati > > ps: cc'ing to ab and al in case the mailing list might reject the huge > attachment -- Albert Chu [EMAIL PROTECTED] 925-422-5311 Computer Scientist High Performance Systems Division Lawrence Livermore National Laboratory _______________________________________________ Freeipmi-devel mailing list Freeipmi-devel@gnu.org http://lists.gnu.org/mailman/listinfo/freeipmi-devel