[PATCH openpower-host-ipmi-oem v3] Fix issue5 for endian safe

[Nan KX Li]

Hi Jeremy,

I've updated my pull request : 
https://github.com/openbmc/openpower-host-ipmi-oem/pull/13.
Please review it.

For the alignment requirement, I'd use unsigned char type to access the 
requestor's buffer. So:

uint8_t *reqptr = (uint8_t *) request; 

esel_req.resid     = le16toh((((uint16_t) reqptr[1]) << 8) + reqptr[0]); 
esel_req.selrecord = le16toh((((uint16_t) reqptr[3]) << 8) + reqptr[2]); 
esel_req.offset    = le16toh((((uint16_t) reqptr[5]) << 8) + reqptr[4]); 



Regards,

William Li ( Li Nan, 李楠 ) 
 

Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605

Beijing, China



From:   Jeremy Kerr <jk at ozlabs.org>
To:     OpenBMC Patches <openbmc-patches at stwcx.xyz>, 
openbmc at lists.ozlabs.org
Cc:     Nan KX Li/China/IBM at IBMCN
Date:   05/30/2016 14:08
Subject:        Re: [PATCH openpower-host-ipmi-oem v3] Fix issue5 for 
endian safe



Hi Nan,

> @@ -29,9 +30,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t 
netfn, ipmi_cmd_t cmd,
>                esel_request_t *reqptr = (esel_request_t*) request;
>                FILE *fp;
>                int r = 0;
> -              // TODO: Issue 5: This is not endian-safe.
> -              short *recid  =  (short*) &reqptr->selrecordls;
> -              short *offset =  (short*) &reqptr->offsetls;
> +              short recid  =  le16toh((((unsigned short) 
reqptr->selrecordms) << 8) + reqptr->selrecordls);
> +              short offset =  le16toh((((unsigned short) 
reqptr->offsetms) << 8) + reqptr->offsetls);

I think that this is less correct than the first attempt, and that's
probably due to an incorrect suggestion from me. Sorry for that.

I've only just noticed that you're using different members of the reqptr
struct to construct that 16-bit value; so your first patch probably did
the right thing.

However: we should fix the actual source of the problem - that the
fields in struct esel_request_t are unnecessarily split into uint8_t
fields.

I think it'd be much better if we changed esel_request_t to this:

  struct esel_request_t {
      uint16_t  resid;
      uint16_t  selrecord;
      uint16_t  offset;
      uint8_t  progress;
  }  __attribute__ ((packed)) ;

- and did the proper conversion with le16toh().

[Also, the typo in the commit title is still present]

Regards,


Jeremy




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160603/2cb02d38/attachment.html>