Re: [PATCH openpower-host-ipmi-oem 2/2] Fix endianness issue5

[Cyril Bur <cyrilbur@gmail.com>]

On Tue, 24 May 2016 04:40:37 -0500
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

> From: Nan Li <bjlinan@cn.ibm.com>
> 
> Make it safe in both little-endian and big-endian bmc chip.
> 

Hi Nan,

Comments below.

> Signed-off-by: Nan Li <bjlinan@cn.ibm.com>
> ---
>  oemhandler.C | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/oemhandler.C b/oemhandler.C
> index 026e0d1..6716618 100644
> --- a/oemhandler.C
> +++ b/oemhandler.C
> @@ -30,8 +30,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  	FILE *fp;
>  	int r = 0;
>  	// TODO: Issue 5: This is not endian-safe.
> -	short *recid  =  (short*) &reqptr->selrecordls;
> -	short *offset =  (short*) &reqptr->offsetls;
> +	unsigned short recid  =  ((unsigned short) reqptr->selrecordls) << 8 + reqptr->selrecordms;
> +	unsigned short offset =  ((unsigned short) reqptr->offsetls) << 8 + reqptr->offsetms;

So, what you're trying to do is have recid and offset be in host cpu endian and
the data that reqptr points to is in a fixed and known endian?

I'm not convinced what you have got this right, in the case where the host and
the data are in the same endian, shouldn't this do nothing, but, regardless,
this will do something...

If you look at endian.h `man 3 endian` it provides you with some very nice
convenience conversion functions which ensure you get it right and make it so
much easier to read and understand the code.

By the looks of it you want something like be16toh() or le16toh().

>  	uint8_t rlen;
>  	ipmi_ret_t rc = IPMI_CC_OK;
>  	const char *pio;
> @@ -59,7 +59,7 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  
>      // OpenPOWER Host Interface spec says if RecordID and Offset are
>  	// 0 then then this is a new request
> -	if (!*recid && !*offset)
> +	if (!recid && !offset)
>  		pio = "wb";
>  	else
>  		pio = "rb+";
> @@ -67,10 +67,10 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  	rlen = (*data_len) - (uint8_t) (sizeof(esel_request_t));
>  
>  	printf("IPMI PARTIAL ESEL for %s  Offset = %d Length = %d\n",
> -		g_esel_path, *offset, rlen);
> +		g_esel_path, offset, rlen);
>  
>  	if ((fp = fopen(g_esel_path, pio)) != NULL) {
> -		fseek(fp, *offset, SEEK_SET);
> +		fseek(fp, offset, SEEK_SET);
>  		fwrite(reqptr+1,rlen,1,fp);
>  		fclose(fp);
>