freeipmi-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anand Avati
Subject: [Freeipmi-devel] Re: bmc-config in C
Date: Wed, 5 Jul 2006 22:00:51 -0700
User-agent: Mutt/1.5.9i

Al,
replies inline.
 
> 1) It seems that both "Password20" and "Password" would be output on a
> bmc-config --checkout.  Only one should be output.

I have an understanding of this requirement only now. I was thinking all
the while that there were two passwords. I shall fix it.
 
> 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";

My bad. I was misled by the fact that there were hardcoded numbers in
the scheme code and I blindly wrote C equivalents. Probably providing
shceme symbols for such names like IPMI_BIT_RATE_9600_BPS defined to 6
would also be a good idea.
 
> 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.

right.
 
> 4)
> 
> Also in k_r_checkout()
> 
> k_r[21] = 0;
> 
> The largest index is 20, so this corrupts memory.  Similarly elsewhere.

yuck, i dont believe i wrote this :p
 
> 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?

A commit wont ignore a blank, It will see if blank is valid via that
key's validate () function (which is valid only for password) and error
out.
 
> 5) I notice throughout almost all the code that you don't check the
> return value of strdup() for == NULL.

Memory allocations fail (atleast in the new kernels) when you run out of 
virtual address. In our situation memory usage is constant, previous
value is free'd and new value is alloc'ed and our memory usage does not
'grow' with time (we dont have an increasing link list or such).
 
In case the system is really running out of physical memeory, your process is
automatically killed by OOM. malloc/strdup never returns NULL for lack
of physical memory (inclusive swap)

> 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?

0 means `autodetect'. It inspects if the initial part of the string is
"0x" or "0" or otherwise and according treats the string as hex, octal
or decimal. That's what my man-page says. (It even says it conforms to
ISO and BSD standards)
 
> 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?

0 is suuccess. I check for 0 or non 0 where i call the validate. 
 
> 8) I also noticed that in character_accumulate_interval_validate() you
> didn't check the range of the actual value.

thanks for pointing. my mistake.
 
> 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)  

I'll do this change, not an issue.
 
> 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.

ok.
 
> 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?

awww, ok ;-)


how do i send the new changes? re-send everything or are you going to
apply the current stuff and i diff against them?

I prefer the latter as it is easy (i dont need to tar new files, diff
old files)

thanks,
avati





reply via email to

[Prev in Thread] Current Thread [Next in Thread]