Re: [Freeipmi-devel] kcs byte array model - validation

[Anand Babu]

,----[ Albert Chu <address@hidden> ]
| Ok, I've looked through it more thoroughly now ... I have an issue
| with the ipmi_get_dev_id_rq function and that you have no declaration
| of a "struct ipmi_get_dev_id_rq_t" type.
`----
For Reference:
~~~~~~~~~~~~~~
    assemble_ipmi_kcs_rq_hdr (IPMI_NET_FN_APP_RQ, IPMI_BMC_IPMB_LUN_BMC, &hdr);
 <= ipmi_get_dev_id_rq (cmd, sizeof(cmd));
 => ipmi_get_dev_id_rq (&rq);
 => marshall_get_dev_id_rq (&rq, cmdbuf, cmdbuflen);
    assemble_ipmi_kcs_rq_pkt (&hdr, cmd, sizeof(cmd), pkt);
    ipmi_kcs_write (pkt, sizeof(pkt));
    ipmi_kcs_read (pkt, sizeof(pkt));
    unassemble_ipmi_kcs_rs_pkt (pkt, sizeof(pkt), &hdr, cmd);
    ipmi_get_dev_id_rs (cmd, sizeof(cmd), cmd_rs);

Reasons why it was implemented so:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- Should we expose marshalling to the user?
- One more extra call per command for the user ?
- Too much work to implement per IPMI command, we have to implement
  hundreds and hundreds of IPMI commands !!

So the change you want is:
~~~~~~~~~~~~~~~~~~~~~~~~~~
1) Implement request structure as well
2) Split
     ipmi_get_dev_id_rq (cmd, sizeof(cmd));
   to
     ipmi_get_dev_id_rq (&rq);
     marshall_get_dev_id_rq (&rq, cmdbuf, cmdbuflen);

,----[ Albert Chu <address@hidden> ]
| 1) The user should not be *REQUIRED* to use the ipmi_get_dev_id_rq()
| function to fill in data within a command structure.  It is only a
| recommended way.  A user, if they'd like to, should be able to fill in
| a structure by themselves, then pass the structure to the marshalling
| function.  For example:
| 
| 2) Similarly, it is also useful to have this method because we don't
| know how IPMI may be different from different vendors.  For example,
| perhaps one vendor has some special OEM feature.  Under my framework,
| we can easily set that feature.  For example:
`----
"abstraction, easy of use"  
   Vs 
"low level interface, greater flexibility"

Your reasons are very valid, especially in the FreeIPMI context.
Mark Grondona made an excellent suggestion of hiding the marshalling
call behind. I meets both of our goals.

'assemble_ipmi_kcs_rq_pkt' call will accept header and command in
struct form, call marshall internally, and return byte array.

,----[ Albert Chu <address@hidden> ]
| Ben says he prefers having the marshalling functions malloc() a buffer
| instead of us passing a buffer in.  I personally prefer passing in a
| buffer and declaring a global macro IPMI_CMD_MAX_LEN.  We can debate
| about this.
`----
I will reply to Ben's email separately. New debate!

,----[ Albert Chu <address@hidden> ]
| I agree with your ipmi_get_dev_id_rs() function.  Although I would
| like macros/constants/whatever to hide the bit masks and stuff ...
`----
Yes we have to. Just waiting for the code to mature and freeze.

,----[ Albert Chu <address@hidden> ]
| unassemble_ipmi_kcs_rs_pkt() ... I think we need a cmdlen parameter.
| Also, since you are specifically passing a ipmi_kcs_hdr_t type, I
| think you should go ahead and unmarshall the header within this
| function, instead of deferring it to another function.  So, I think it
| should be more like:
`----
It is a BUG. I did that because hdr is just 1 byte long. I already
fixed it. We should have no traces of memcpy/struct in our code any
more.

We have netfn2byte, byte2netfn functions. I should probably rename it
to marshall_netfn, unmarshall_netfn to make it look more consistent.

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