[Freeipmi-devel] Re: [llnl-devel] New FreeIPMI API thouoghts

[Anand Babu]

Albert Chu <address@hidden> writes:
>You seem to have an "inband IPMI core" and some "LAN wrapper" code
>around it for IPMI over LAN.  Well, why not just make the "LAN
>wrapper" code around the "inband IPMI core" more complete rather than
>just the "partial" wrapper you have right now.  i.e.
>
>typedef struct {
>   ipmi_session_t sess;
>   ipmi_request_t rq;
>   ipmi_get_session_challenge_rq_t cmd;
>   u_int8_t chksum;
>} ipmi_lan_get_session_challenge_rq_t;
>
>ipmi_lan_get_session_challenge_rq(ipmi_lan_get_session_challenge_rq_t *rq,
>                                  u_int8_t foo,
>                                  u_int8_t bar,
>                                  etc...) {
>     ipmi_session(&(rq->sess), foo);
>     ipmi_request(&(rq->rq), bar);
>     ipmi_get_session_challenge_rq(&(rq->cmd));
>     rq->chksum = calculate_checksum(&(rq->rq.rs));
>                                      ^^ whatever the field is, 
>                                         I can't remember.
>}
>
>That way, we can make the IPMI over LAN API almost identical to the
>IPMI inband API.  I think it would be particularly nice to have the
>APIs similar to each other.  The only difference is:
>  - Do you use "ipmi" or "ipmi_lan" to create packets.
>  - Do you use "ipmi_lan" or "ipmi_kcs" or "ipmi_smi" to send/recv.
>
>Several other reasons I think the above would be better:
> - Right now there is no (clean) way to access any fields in an 
>   ipmi-request packet except for the data portion.
> - This hides the rmcp_hdr_t, ipmi_session_t, and ipmi_response_t 
>   structures from the user, so they don't need to bother with them 
>   in the unassemble() functions.  They can just use the macros and
>   the same pointer to check/get everything.
>
>The only downside (that I can see) of this approach is suppose someone
>wants to use FreeIPMI and write a piece of software that does IPMI
>inband on the localhost and IPMI LAN to all other nodes.  Then perhaps
>they cannot use as much duplicate code.  But, the code's is already
>going to have a "special case" for the localhost anyways.
>
>Let me know what you think.  This is something I thouoght about a lot
>and am still thinking about.  The overall point is I think the API can
>be cleaner.

I tried this approach and dropped it. 
We will have to maintain at least 8 structures and 3 functions for
every command. This will soon become a nightmare. Remember we have not
thought about bt or serial (basic, PPP) interfaces yet.


For example GET_SESSION_CHALLENGE COMMAND:

 - ipmi_get_session_challenge_cmd_rq_t // common command struct
 - ipmi_lan_get_session_challenge_rq_t
 - ipmi_smic_get_session_challenge_rq_t
 - ipmi_kcs_get_session_challenge_rq_t
 - ipmi_get_session_challenge_cmd_rs_t // common command struct
 - ipmi_lan_get_session_challenge_rs_t
 - ipmi_smic_get_session_challenge_rs_t
 - ipmi_kcs_get_session_challenge_rs_t


and
 - ipmi_lan_get_session_challenge_rq (...)
 - ipmi_smic_get_session_challenge_rq (...)
 - ipmi_kcs_get_session_challenge_rq (...)


If you look at the current implementation, it is much simpler. 2
structs per command request. 

 - ipmi_cmd_get_session_challenge_rq_t
 - ipmi_cmd_get_session_challenge_rs_t

 SHARED GENERIC FUNCTIONS FOR ALL COMMANDS.
 assemble_lan_pkt ()        
 unassemble_lan_pkt ()
 assemble_kcs_pkt ()
 unassemble_kcs_pkt ()
 ..
 ipmi_lan_sendto
 ipmi_kcs_write
 ipmi_smic_write
 ...
 

>2)
>
>int assemble_ipmi_lan_pre_session_rq_pkt (net_fn_t net_fn, u_int8_t
>rq_seq, void *ipmi_lan_cmd, u_int32_t ipmi_lan_cmd_len, void
>*ipmi_lan_pkt, u_int32_t ipmi_lan_pkt_len);
>
>It seems strange to pass in a net_fn_t parameter to the assemble functions.
>
>Why don't we just make two parameters:
>
>u_int8_t net_fn, u_int8_t lun
>
>So we can hide the details of the net_fn_t type from the user.  The
>user doesn't need to know about the net_fn_t type, so there's no
>reason to make them use it.

That was just to reduce the number of arguments. But it makes sense to
split them. You want to do this?

>3)
>
>unassemble_ipmi_lan_pre_session_rs_pkt (void *ipmi_lan_pkt, u_int32_t
>ipmi_lan_pkt_len, u_int32_t ipmi_lan_cmd_len, rmcp_hdr_t *rmcp_hdr,
>ipmi_session_t *session, ipmi_lan_msg_rs_t *msg, void *ipmi_lan_cmd,
>ipmi_chksum_t *chksum)
>
>Seems like we're missing an ipmi_lan_cmd_len parameter.

Look at the 3rd argument. As a convention, all input arguments are
placed first.

>4)
>
>I think we should stay consistent between the IPMI packets and the
>RMCP ping/pong packet structures.  If we're going to have RMCP packets
>within the ipmi request packets, we should have rmcp hdrs within the
>rmcp_ping structure too.  I don't know what would be best, but we
>should be consistent.  That's why I liked the old ipmi_sendto API with
>passing in both the rmcp_hdr_t * and the void*buf.

I removed rmcp_hdr from rmcp_ping structure to make it consistent with
the IPMI command structures. 

Let us discuss more about this today when we meet.

-- 
Anand Babu
CaliforniaDigital.com
Office# +1-510-687-7045
Cell# +1-510-396-0717
Home# +1-510-894-0586

Free as in Freedom <www.gnu.org>