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

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

[OpenBMC Patches]

Make it safe in both little-endian and big-endian bmc chip.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/openbmc/openpower-host-ipmi-oem/13)
<!-- Reviewable:end -->


https://github.com/openbmc/openpower-host-ipmi-oem/pull/13

Nan Li (2):
  Handle multiple attempts at ipmi reservation ids
  Fix endianness issue5

 oemhandler.C | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

-- 
2.8.3

[PATCH openpower-host-ipmi-oem 1/2] Handle multiple attempts at ipmi reservation ids

[OpenBMC Patches]

From: Nan Li <bjlinan@cn.ibm.com>

1. Check Reservation ID.
2. Use Reservation ID from shared librarys.

Signed-off-by: Nan Li <bjlinan@cn.ibm.com>
---
 oemhandler.C | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/oemhandler.C b/oemhandler.C
index 22452d6..026e0d1 100644
--- a/oemhandler.C
+++ b/oemhandler.C
@@ -10,7 +10,6 @@ void register_netfn_oem_partial_esel() __attribute__((constructor));
 const char *g_esel_path = "/tmp/esel";
 uint16_t g_record_id = 0x0001;
 
-
 ///////////////////////////////////////////////////////////////////////////////
 // For the First partial add eSEL the SEL Record ID and offset
 // value should be 0x0000. The extended data needs to be in
@@ -29,6 +28,7 @@ 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;
@@ -36,7 +36,28 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 	ipmi_ret_t rc = IPMI_CC_OK;
 	const char *pio;
 
-	// OpenPOWER Host Interface spec says if RecordID and Offset are
+	unsigned short used_res_id = 0;
+	unsigned short req_res_id = 0;
+
+	used_res_id = get_sel_reserve_id();
+
+	req_res_id = (((unsigned short)reqptr->residms) << 8) + reqptr->residls;
+
+	// According to IPMI spec, Reservation ID must be checked.
+	if ( used_res_id != req_res_id ) {
+		// 0xc5 means Reservation Cancelled or Invalid Reservation ID.
+		printf("Used Reservation ID = %d\n", used_res_id);
+		rc = IPMI_CC_INVALID_RESERVATION_ID;
+
+		// clean g_esel_path. 
+		r = remove(g_esel_path);
+		if(r < 0)
+			fprintf(stderr, "Error deleting %s\n", g_esel_path);
+
+		return rc;
+	}
+
+    // OpenPOWER Host Interface spec says if RecordID and Offset are
 	// 0 then then this is a new request
 	if (!*recid && !*offset)
 		pio = "wb";
-- 
2.8.3

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

[OpenBMC Patches]

From: Nan Li <bjlinan@cn.ibm.com>

Make it safe in both little-endian and big-endian bmc chip.

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

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

[Cyril Bur]

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