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

[Jeremy Kerr <jk@ozlabs.org>]

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