[Freeipmi-devel] New FreeIPMI API thouoghts

[Albert Chu]

Hey AB,

Here are some thouoghts:

1)

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.


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.

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.

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 will probably notice more things in the future.

Al

--
Albert Chu
address@hidden
Lawrence Livermore National Laboratory1)