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

[Albert Chu]

Hey Anand,

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

I hope you haven't started or gotten too far in this fix.  I just thought
of (IMO) a better solution (I don't know why it took me so long).

It's very common for users to do

/usr/sbin/bmc-config --checkout > foofile.
edit 1 tiny thing in foofile
/usr/sbin/bmc-config --commit -f foofile

then, b/c the default checkout is to leave Password blank (b/c you can't
check it out), you've now accidently overwritten your default passwords.

So instead, how about the default checkout for Password and Password20
have those keys automatically commented out?  like

# instructions for Password
# Password
# instructions for Password20
# Password20

Since Password is the only one that cannot be checked out (I think, I
can't recall on K_R and K_G), I think the above makes the best sense.  It
will allow us to output both Password and Password20, letting the user
know they can do one of either one, and not mess up recommits.

I suppose there has to be one more additional check that has to make sure
the user doesn't input two passwords too.

I can't recall if you can checkout k_r or k_g keys.  If we can't, then the
above also holds for k_r and k_g.

Al

>
> 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
>
>
>
> _______________________________________________
> Freeipmi-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/freeipmi-devel
>


-- 
Albert Chu
address@hidden
925-422-5311
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory