[Freeipmi-devel] Re: bmc-config in C

[Al Chu]

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
address@hidden
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory