RE: [Freeipmi-devel] opinion on new ipmi_kcs_cmd_raw()

[Cress, Andrew R]

Al,

This hits the mark for what I was hoping to see.  
Some implementation comments:

Why have two separate buffers for the header and data, then combine into
a third buffer?
I think it would be better to allocate enough space for the total, then
fill the header first, then copy the data after it.

I have some other comments, labelled /*ARC: */ below.

Andy

-----Original Message-----
From: address@hidden
[mailto:address@hidden On
Behalf Of Albert Chu
Sent: Wednesday, November 10, 2004 1:34 PM
To: address@hidden
Subject: [Freeipmi-devel] opinion on new ipmi_kcs_cmd_raw()


Just want to gather opinions on this.  ipmi_kcs_cmd_raw_interruptible
would be similar.

int8_t
ipmi_kcs_cmd_raw (u_int16_t sms_io_base, u_int8_t lun, u_int8_t fn,
u_int8_t *buf_cmd_rq, u_int32_t buf_rq_len, u_int8_t *buf_cmd_rs,
u_int32_t *buf_rs_len)
{
  if (!(sms_io_base && buf_cmd_rq && buf_rq_len > 0
        && buf_cmd_rs && buf_rs_len && *buf_rs_len > 0))
    {
      errno = EINVAL;
      return (-1);
    }
                                                                       
            
  { /* Request Block */
    fiid_obj_t obj_hdr_rq = NULL;
    u_int8_t *bytes = NULL;
    u_int32_t obj_hdr_rq_len, bytes_len;
                                                                       

    obj_hdr_rq_len = fiid_obj_len_bytes (tmpl_hdr_kcs);
    ERR (obj_hdr_rq_len > 0);
    obj_hdr_rq = alloca (obj_hdr_rq_len);
    memset (obj_hdr_rq, 0, obj_hdr_rq_len);     /*ARC: move after
ERR(obj_hdr_rq) */
    ERR (obj_hdr_rq);                   
    ERR (fill_hdr_ipmi_kcs (lun, fn, obj_hdr_rq) != -1);
                                                                       
            
    bytes_len = obj_hdr_rq_len + buf_rq_len;
    bytes = alloca (bytes_len);
    memset (bytes, 0, bytes_len);  /*ARC: move after ERR(bytes) */
    ERR (bytes);
                                                                       
            
    memcpy(bytes, obj_hdr_rq, obj_hdr_rq_len);
    memcpy(bytes + obj_hdr_rq_len, buf_cmd_rq, buf_rq_len);
                                                                       
            
    ERR (ipmi_kcs_write (sms_io_base, bytes, bytes_len) != -1);
  }
  { /* Response Block */
    fiid_obj_t obj_hdr_rs = NULL;
    u_int8_t *bytes = NULL;
    u_int32_t obj_hdr_rs_len, bytes_read, bytes_len;
                                                                       
            
    obj_hdr_rs_len = fiid_obj_len_bytes (tmpl_hdr_kcs);
    ERR (obj_hdr_rs_len != -1);
    obj_hdr_rs = alloca (obj_hdr_rs_len);
    memset (obj_hdr_rs, 0, obj_hdr_rs_len);  /*ARC: move after ERR()*/
    ERR (obj_hdr_rs);
                                                                       
            
    bytes_len = obj_hdr_rs_len + *buf_rs_len;
    bytes = alloca (bytes_len);
    memset (bytes, 0, bytes_len);
    ERR (bytes);
                                                                       
            
    ERR ((bytes_read = ipmi_kcs_read (sms_io_base, bytes, bytes_len)) !=
-1);
    if (bytes_read > obj_hdr_rs_len)
      {
        u_int32_t rs_len = bytes_read - obj_hdr_rs_len;
        if (rs_len <= *buf_rs_len)
          *buf_rs_len = rs_len;
        /* else leave buf_rs_len the same size */ 
 /*ARC: Never gets to else, since bytes_len limits bytes_read, based on
*buf_rs_len, 
        so drop the if(rs_len <=) condition. */
                                                                       
            
        memcpy(buf_cmd_rs, bytes + obj_hdr_rs_len, *buf_rs_len);
      }
    else  
      /*ARC: Still need to return the cmd byte and comp_code here, if
(bytes_read > 0) */
      *buf_rs_len = 0;
  }
  return (0);
}


--
Albert Chu
address@hidden
Lawrence Livermore National Laboratory



_______________________________________________
Freeipmi-devel mailing list
address@hidden
http://lists.gnu.org/mailman/listinfo/freeipmi-devel