[Freeipmi-devel] Re: [llnl-devel] FreeIPMI conflict/issue list

[Anand Babu]

Albert Chu <address@hidden> writes:
>I've gotten the chance to look through the FreeIPMI code. Below is a
>list of framework/architecture/code issues I've found.  Please let me
>know your thoughts on the conflicts below.  Assuming I can checkin to
>CVS, I will begin making changes in CVS for non-conflicting views or bug
>fixes.
Yes you can checkin your code. You have write access to CVS and part
of FreeIPMI core team. You will create a new sub project - ipmipower.
Let me know if you need help to understand the savannah.gnu.org of CVS
and project interface.

>Also, when we were at CDC, I wrote a number of things on the white board
>that I felt were necessary for the library.  They included: functions to
>check response packet checksums, completion codes, etc., packet
>fprintf/snprintf/perror type functions for debugging, chassis status

Like I said, The code is already in there. But some of them do not
have higher level macros or function calls.

CHECK RESPONSE CODE and ipmi_strerror_r (instead of perror):
------------------------------------------------------------
    if (chassis_ctrl_rs.msg.rs.comp_code != IPMI_COMMAND_SUCCESS)
      {
        char errstr[IPMI_ERR_STR_MAX_LEN];
        ipmi_strerror_r (IPMI_CMD_CHASSIS_CTRL, 
chassis_ctrl_rs.msg.rs.comp_code, errstr, IPMI_ERR_STR_MAX_LEN);
        fprintf (stderr, "ipmi_chassis_ctrl_rs: %s\n", errstr);
        return (-1);
      }

Refer to ipmi-errno.h and ipmi-errno.c for complete doc on error codes

Should libfreeipmi include higher level calls? 
Usually different applications would prefer to print in different
style. May be GTK or console interface. It is best to just return the
error string to user and then let him decide how to print. 

CHECKSUM:
---------
    if (set_session_priv_level_rs.chksum !=
        ipmi_chksum ((u_int8_t *) &set_session_priv_level_rs.msg.rs,
                     (sizeof (set_session_priv_level_rs.msg.rs) + 
                      sizeof (set_session_priv_level_rs.data)))) 
      {
        fprintf (stderr, "ipmi_set_session_priv_level_rs: chksum mismatch\n");
        return (-1);
      }

We can write a simple macro around this. Because the structs are
consistent across the code and the command specific fields 
are abstracted with known block names, it should work.

#define IPMI_CHKSUM (rs_block) \
 do { \
    if (rs.chksum != ipmi_chksum ((u_int8_t *) &rs_block.msg.rs, \
                     (sizeof (rs_block.msg.rs) + sizeof (rs_block.data)))) \
      { \
        return (1); \
      } \
    return (0); \
 } while (0)

What do you think?

>command (22.2 of the specification), and functions to gather field
>values from packets.  I believe you said they were completed, but I
>haven't been able to find a number of these functions.  Have they been
>completed yet?  Possibly forgotten to be checked into CVS?

You can find all higher level calls and examples under a single file
fish/src/ipmi-wrapper.c (which may fork as libfish)

I'm supposed to write about all these and help you understand the
code in less time. My previous mail is still in my draft folder and
got sucked into other activities. 

>Stupid bugs (I can fix these, they are trivial)
>-----------------------------------------------
You are already in core team. You don't need other's vote to fix bugs
and make changes directly on the CVS source that will not break
dependent applications or framework related issues. Jim and you have a
good understanding how to drive this project forward.

>ipmi_activate_session_rq
>- IPMI_SESSION_MAX_USERNAME_LEN should be IPMI_CHALLENGE_STRING_LEN or
>whatever.

#define IPMI_SESSION_MAX_USERNAME_LEN  0x10
#define IPMI_SESSION_CHALLENGE_STR_LEN 0x10

Both of these macros exist in libfreeipmi/src/ipmi-msg-support-cmds.h.
Username is required for ipmi_get_session_challenge_rq and 
challenge_str for ipmi_get_session_challenge_rs.

It is possible in future, username or password may have varying
lengths. Challenge string field is also used as password field based
on session settings.

>Framework/API/Architecture Conflicts
>------------------------------------
>session_seq_num = initial_inbound_seq_num + rq_seq;
>- Your framework seems to assume the requester sequence number is used 
>  as a sequential sequence number that should be added to the initial 
>  inbound sequence number.  I don't believe this is a valid assumption.  
>  The requester sequence number can be used for any purpose by the user.
>  Instead, I think there should be a parameter that we pass into the   
>  packet creation function for "inbound count" or "inbound count 
>  increment" or something.
>
>  In addition, since the requester sequence number is only 6 bits, the
>  sequence number can wrap around very quickly and will cause problems 
>  when it is used when keeping sessions open and active.  If I send 
>  "pings" every 10 seconds to keep a LAN session open, I will wrap 
>  around after 20 minutes.

You are absolutely correct. Thanks for pointing it out. 

,----
|   ipmi_chassis_ctrl_rq (u_int8_t auth_type, u_int32_t
|   initial_inbound_seq_num, u_int32_t session_id, u_int8_t *auth_code,
|   u_int8_t rq_seq, u_int8_t chassis_ctrl, ipmi_chassis_ctrl_rq_t
|   *chassis_ctrl_rq) 
|    
|   chassis_ctrl_rq->session.session_seq_num = initial_inbound_seq_num +
|   rq_seq;
`----
 
should be changed as 

,----
|   ipmi_chassis_ctrl_rq (u_int8_t auth_type, u_int32_t
|   inbound_seq_num, u_int32_t session_id, u_int8_t *auth_code,
|   u_int8_t rq_seq, u_int8_t chassis_ctrl, ipmi_chassis_ctrl_rq_t
|   *chassis_ctrl_rq) 
|    
|   chassis_ctrl_rq->session.session_seq_num = inbound_seq_num;
`----

>IPMI_DEFAULT_LUN
>- I think it would be better to have a lun function parameter in the
>  packet creation functions, where we can pass IPMI_BMC_LUN or
>  IPMI_OEM_LUN or whatever.  Is it wise to assume users always want to
>  use IPMI_DEFAULT_LUN??

Correct, make no assumptions while writing libraries. We should add
another arg for LUN. 

>ipmi_sendto
>- gethostbyname is not thread safe.  I think it would be better to
>  have ipmi_sendto take a 'struct sockaddr_in' as a parameter
>  instead of a hostname.

I was thinking about it. It is best for the application to resolve the
host names than the library. Should be changed to struct sockaddr_in.
Also it is very important to have libfreeipmi thread-safe.


>Port Issues
>------------
>
>I also used #pragma pack(1) in my library.  However, I don't believe
>this can be considered a long term solution.  Besides machine/compiler
>compatability issues, there are compilation issues that can arise when
>using the library.  For example, if one library packs on a normal 8
>byte boundary (assume a libfoo, with a 'struct foo' type), then
>compilation issues can arise if we do.
>
>#include <foo.h>
>#include <freeipmi.h>
>
>struct newtype {
>    struct freeipmi_pkt p;
>    struct foo f;
>};
>
>newtype includes two structures that two different libraries believe
>are packed differently.  This is a trivial example that may be
>caught and fixed by a compiler, but I have encountered non-trivial
>instances in ipmipower where alignment causes major problems.
>
>I also did the same as you did with freeipmi in regards to
>sendto/recvfrom.  I just pass the the address of a struct ipmi_pkt as a
>char * pointer to sendto/recvfrom.  However, there are
>compiler/porting/endian issues with this method.  I think we need a set of:
>
>marshall_ipmi_pkt(struct ipmi_pkt *, char *buffer, char *len);
>unmarshall_ipmi_pkt(struct ipmi_pkt *, char *buffer, char *len);
>
>functions to deal with these issues.  By having marshalling and
>unmarshalling functions, we can also solve the #pragma pack(1) issue.
>
>I think we can leave #pragma pack(1) and the sendto/recvfrom framework
>for now because they work, but this is something I feel must be changed
>in the future.

Definitely you pointed out a bug. 

   There are three approached to packing structure.
   => "-fpack-struct" in src/Makefile.am
   => PACKED marco for __attribute__((packed)) for every struct
   => #pragma pack(1)

Clearly #pragma pack approach is the best of all. 

My fix would be to add
#pragma pack(0)
at the end of header files which calls #pragma pack(1)
This is the usual way it is done. But I'm not sure how compiler would
treat #pragma pack inside ipmi_{sendto, recvfrom}. Help me understand
which approach will do the job.

Another change is to move the chassis control related code to
src/ipmi-chassis-cmd.{c, h} from ipmi-msg-support-cmds.c

I may move the session headers out like I did for rmcp headers from
these structures after I checkin my user-space IPMI driver.

Another interesting info is.
We should be able to meet the error handling (that MCA is supposed to
do through IPMI itself). By writing a simple panic handler for system
management interrupt. BMC also puts the system in system management
mode after raising the interrupt. We can safely throw info to console
and record errors in SEL.

Desmond Yuen and Bryan Rice pointed excellent references that
addresses most of our s/w needs. I will write about it in detail in my
next email. Thanks Des and Bryan.

I will be able to wrap up including documentation for most of the
tasks we have, in a month. Left out will be long term inband BIOS/FRU/BMC
upgrade utility. Intel may release this utility as free software when
they make the new BIOS release (may be end of December).

Happy Hacking
-- 
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>