[Freeipmi-devel] FreeIPMI conflict/issue list

[Albert Chu]

Hey AB, 

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.

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
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?

Thanks,
Al

Stupid bugs (I can fix these, they are trivial)
-----------------------------------------------

ipmi_activate_session_rq
- IPMI_SESSION_MAX_USERNAME_LEN should be IPMI_CHALLENGE_STRING_LEN or
whatever.

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.

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??

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.

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.