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

[Albert Chu]

> Mark Grondona made an excellent suggestion of hiding the marshalling
> call behind. I meets both of our goals.

This is what I considered in the beginning, but I felt it would be
a little messier when we do unassemble.  The unassemble function would
be something like this:

unassemble_pkt(ipmi_hdr_t *hdr, void *cmd, void*buf, int buflen) {
  
   unmarshall_hdr(hdr, buf, buflen);
   buf += HDR_LEN;  /* whatever, you get the idea */
   
   if (buf[0] == IPMI_FOO_PKT_TYPE)
      struct foo_type *foo = (foo_type *)cmd;
      foo->val1 = buf[0];
      foo->val2 = buf[1] & MY_SPECIAL_MASK;
      foo->val3 = buf[1] & MY_SPECIAL_MASK2; 
   }
}

The reason I don't like this is because what should we do cmd is a
pointer to type FOO, and buf[0] is packet type BAR??  
Do we pass in "type" id that tells us what kind of pointer "cmd" is??
Do we write an unassemble routine for each command type??  Do we ignore
the packet if it isn't the type we're expecting?? 

I just thought of this, maybe it would be better to malloc() within
the unassemble function, and return a pointer to a buffer containing
the unassembled/unmarshalled packet??

if (buf[0] == IPMI_FOO_PKT_TYPE)
   struct foo_type *foo = malloc(sizeof(struct foo_type));
   foo->val1 = buf[0];
   foo->val2 = buf[1] & MY_SPECIAL_MASK;
   foo->val3 = buf[1] & MY_SPECIAL_MASK2; 
   return foo;
}

I dunno, we're getting into some funny stuff now ... 

At the same time, there is nothing that says we *have* to have the same
style with our assemble and unassemble functions.  We can have them
different ... 

Let me think about it a bit .. I'm just sort of rambling now ..

Al

--
Albert Chu
address@hidden
Lawrence Livermore National Laboratory

----- Original Message -----
From: Anand Babu <address@hidden>
Date: Thursday, December 11, 2003 3:55 pm
Subject: Re: [Freeipmi-devel] kcs byte array model - validation

> ,----[ 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>
>