freeipmi-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Albert Chu
Subject: Re: [Freeipmi-devel] kcs byte array model - validation
Date: Wed, 10 Dec 2003 08:55:09 -0800

Hey AB,

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.  

In the function ipmi_get_dev_id_rq, you copy data directly into the
buffer, and not a structure type.  I talked to Ben about this yesterday,
and he agrees with me that it should be more like this:

ipmi_kcs_hdr_t hdr;
ipmi_get_dev_id_rq_t rq;
ipmi_get_dev_id_rs_t rs;
char cmdbuf[CMDBUFLEN];
char pktbuf[PKTBUFLEN];

assemble_ipmi_kcs_rq_hdr(&hdr);  /* fill in contents of kcs header */
ipmi_get_dev_id_rq (&rq);        /* fill in contents of request */

/* marshall request struct into a cmdbuf */
cmdlen = marshall_get_dev_id_rq (&rq, cmdbuf, CMDBUFLEN);

/* marshall header into pktbuf, and memcpy cmdbuf to end of pktbuf. */
assemble_ipmi_kcs_rq_pkt (&hdr, cmdbuf, cmdlen, pktbuf, PKTBUFLEN);

There are several reasons for this framework vs. yours.

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:

assemble_ipmi_kcs_rq_hdr(&hdr);
rq.function = IPMI_CMD_FUNCTION_ID;
rq.data = IPMI_DO_A_JOB_ID;
rq.val = 0x08;
rq.checksum = calculate_checksum_somehow();
cmdlen = marshall_get_dev_id_rq (&rq, cmdbuf, CMDBUFLEN);
assemble_ipmi_kcs_rq_pkt (&hdr, cmdbuf, cmdlen, pktbuf, PKTBUFLEN);

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:

assemble_ipmi_kcs_rq_hdr(&hdr);
ipmi_get_dev_id_rq (&rq);
rq.reserved = IPMI_DO_SPECIAL_OPERATION;
rq.reserved2 = IPMI_DO_SPECIAL_THINGIE_TOO;
cmdlen = marshall_get_dev_id_rq (&rq, cmdbuf, CMDBUFLEN);
assemble_ipmi_kcs_rq_pkt (&hdr, cmdbuf, cmdlen, pktbuf, PKTBUFLEN);

However, your framework does not allow this as easily.

3)

Ummm, I had just thought of something, but then I lost it ... maybe it
will come back to me later.

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 agree with your ipmi_get_dev_id_rs() function.  Although I would like
macros/constants/whatever to hide the bit masks and stuff ...

assemble_ipmi_kcs_rq_pkt() ... This is exactly what I was thinking
in terms of "marshalling" ... 

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:

u_int8_t
unassemble_ipmi_kcs_rs_pkt (u_int8_t *pkt, u_int8_t pkt_len,
ipmi_kcs_hdr_t *hdr, u_int8_t *cmd)
{
  if ((hdr == NULL)
      || (cmd == NULL)
      || (pkt == NULL)
      || (pkt_len < IPMI_KCS_HDR_LEN))
    {
      errno = EINVAL;
      return -1;
    }
  
  hdr->net_fn.fn = (pkt[0] & NETFN_MASK) >> 2;
  hdr->net_fn.lun = (pkt[0] & LUN_MASK);
  memcpy (cmd, pkt + IPMI_KCS_HDR_LEN, pkt_len - IPMI_KCS_HDR_LEN);
  return 0;
}

Al

--
Albert Chu
address@hidden
Lawrence Livermore National Laboratory

----- Original Message -----
From: Anand Babu <address@hidden>
Date: Tuesday, December 9, 2003 6:58 pm
Subject: Re: [Freeipmi-devel] kcs byte array model - validation

> Take a look at ipmi-dev-global.{c,h} as a model. I have byte-array 
> modelimplemented for other commands. Yes, LAN part is untouched as 
> of now.
> After your review, I will go ahead and integrate them into the
> libfreeipmi framework.
> -ab
> 
> ,----[ Albert Chu <address@hidden> ]
> | I've only begun skimming through the code now.  I'll send a more
> | thorough e-mail tomorrow.  But just to make sure.  It doesn't see 
> like| you added any marshalling/unmarshalling to anything LAN 
> related.  You
> | only want me to look at the byte array stuff you did for KCS?  
> Then we
> | would follow the model with LAN??
> `----
> 
> --
> Albert Chu
> address@hidden
> Lawrence Livermore National Laboratory
> 
> ----- Original Message -----
> From: Anand Babu <address@hidden>
> Date: Tuesday, December 9, 2003 4:36 pm
> Subject: Re: [Freeipmi-devel] kcs byte array model - validation
> >
> >> 
> >> ,----[ Albert Chu <address@hidden> ]
> >> | The kcs-test.c file seems to be missing ...
> >> `----
> >> Sorry, Got kcs-test.c overwrote the libfreeipmi tar ball. 
> >> Fixed it now.
> >> -ab
> >> 
> >> ----- Original Message -----
> >> From: Anand Babu <address@hidden>
> >> Date: Tuesday, December 9, 2003 3:56 pm
> >> Subject: [Freeipmi-devel] kcs byte array model - validation
> >> >
> >> >> 
> >> >> New byte-array model implemented for inband user-space IPMI KCS
> >> >> driver. Please evaluate this framework tell me your opinion.
> >> >> 
> >> >> ftp://ftp.cdclinux.com/pub/ipmi/libfreeipmi-0.0.0-kcs-
> byte.tar.gz>> >> ftp://ftp.cdclinux.com/pub/ipmi/kcs-test.c
> >> >> 
> >> >> Later today, I will cleanup the code and release inband
> >> >> bmc-configuration utility too.
> >> >> 
> >> >> "get_dev_id" few fields may not show correct values. I have 
> fixed>> >> inside my repository. Just use this code for design 
> evaluation.>> >> 
> >> >> Once, everyone approves, I will go ahead and integrate SMIC 
> >> driver and
> >> >> rewrite LAN driver.
> >> >> 
> >> >> -- 
> >> >> 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>
> >> >> 
> >> >> 
> >> >> _______________________________________________
> >> >> Freeipmi-devel mailing list
> >> >> address@hidden
> >> >> http://mail.nongnu.org/mailman/listinfo/freeipmi-devel
> >> >> 
> >> >
> >> 
> >> -- 
> >> 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>
> >> 
> >
> 
> -- 
> 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>
> 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]