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

[Albert Chu]

> Why have two separate buffers for the header and data, then combine 
> intoa third buffer?

Because I copied from ipmi_kcs_cmd() of course :-), lots should be fixed.

>    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);

AB or Bala, do you know why a lot of the code has the ERR check after
the memset?  I remember wanting to change this in a lot of places a long
time ago, but I convinced myself I shouldn't.  I can't remember why.  On
Linux, the behavior of alloca() is undefined on error, so that may have
been why I chose not to change anything.  But a NULL check would still
be correct.

Al

--
Albert Chu
address@hidden
Lawrence Livermore National Laboratory

----- Original Message -----
From: "Cress, Andrew R" <address@hidden>
Date: Wednesday, November 10, 2004 2:03 pm
Subject: RE: [Freeipmi-devel] opinion on new ipmi_kcs_cmd_raw()

> 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 
> intoa third buffer?
> I think it would be better to allocate enough space for the total, 
> thenfill 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
>