[Freeipmi-devel] Re: [llnl-devel] checking chksums, possible framework change needed??

[Anand Babu]

Albert Chu <address@hidden> writes:

>> Validity of the comp_code itself depends on the chksum. But location
>> of chksum depends on comp_code. There seems to be a cyclic dependency.
>
> I don't really think this is true.  The chcksum is always at a known
> location based on the length of the packet read.  In fact, both chksum
> values (the one for the response data, and the one for the slave address
> & netfn) are both at known locations which can be easily calculated.

Refer to Figure 12.4 IPMI LAN Message Format:
Request data bytes before the second chksum can be 0 or more bytes. I
was initially under the assumption that they will be at a fixed
address for a particular command. But they don't seem to be. Even You
have noticed response data bytes varying based on the comp_code. 

Problem is not locating the chksum (we can use your last-byte
assumption), but to calculate the chksum locally to match. This
depends on comp_code.

>> I think it is OK, not to validate the chksum if comp_code is not
>> success. In that case we can check comp_code first.
>
> This may be ok from an IPMI standpoint, but I don't think its ok from a
> library standpoint.  A user that uses the FreeIPMI library (I would
> think) expects a function or macro in which it can pass a buffer and
> know that the function can calculate the checksum correctly.  I don't
> think we can expect a user to know the order in which to do packet checks.

Yes correct. We should write a macro to abstract these details and
move them into libfreeipmi 

>> We can double-check the chksum and then validate comp_code
>> like this
>
> I can't help but find this a little hack-ish.  I think it would be fine
> if we hid this within a set of functions and macros.  But I don't think
> this is something a user of FreeIPMI should expect.

Correct, It is a hack and should be abstracted by libfreeipmi.

> By the way, I am coming upon another issue.  I think it would be good
> to have a set of macros such as:
>
> check_net_fn_application_response(ptr)
>    ptr->foo->netfn.fn == APPLICATION_NETFN_RESPONSE ? 1 : 0;
>
> or
>
> check_completion_code_normal(ptr)
>    ptr->foo->code == NORMAL_COMPLETION ? 1 : 0.
>
> But by doing so, I remove a lot of the modularity you have put into the
> library.  These macros require knowledge of more than 1 module
> ipmi-msg-support-cmds.h/ipmi-chassis-cmd.h and
> ipmi-error.h/ipmi-netfn.h/ipmi-cmd-spec.h.  It will get wierder when/if
> we start writing macros to check checksums, b/c we'll need the code fro
> ipmi-utils.[ch] too.
>
> I'm thinking perhaps we should put all of the macros into a new file,
> and only that one file will load all the .h files it needs?? Or perhaps
> we could put the macros all n freeipmi.h??  What do you think??

Both freeipmi.h and ipmi-utils.h are good place to hold macros to
check completion code kind of macros.

Between them, I will choose ipmi-utils.h. They are basically utility
functions/macros that aid application programs. They directly do not
implement the spec.

Code is freezing now. It is good time to start writing macros before
it gets too late. 

Happy Hacking
-- 
 _.|_ 
(_||_)
Free as in Freedom <www.gnu.org>