* [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
@ 2016-05-26 2:00 OpenBMC Patches
2016-05-26 2:00 ` OpenBMC Patches
0 siblings, 1 reply; 6+ messages in thread
From: OpenBMC Patches @ 2016-05-26 2:00 UTC (permalink / raw)
To: openbmc
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 (1):
Fix endianness issue5
oemhandler.C | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--
2.8.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
2016-05-26 2:00 [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5 OpenBMC Patches
@ 2016-05-26 2:00 ` OpenBMC Patches
2016-05-26 2:25 ` Jeremy Kerr
0 siblings, 1 reply; 6+ messages in thread
From: OpenBMC Patches @ 2016-05-26 2:00 UTC (permalink / raw)
To: openbmc; +Cc: Nan Li
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 | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/oemhandler.C b/oemhandler.C
index 026e0d1..2ec004d 100644
--- a/oemhandler.C
+++ b/oemhandler.C
@@ -29,9 +29,9 @@ 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;
+
+ unsigned short recid = ((unsigned short) reqptr->selrecordms) << 8 + reqptr->selrecordls;
+ unsigned short offset = ((unsigned short) reqptr->offsetms) << 8 + reqptr->offsetls;
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
2016-05-26 2:00 ` OpenBMC Patches
@ 2016-05-26 2:25 ` Jeremy Kerr
2016-05-26 3:17 ` Nan KX Li
2016-05-26 3:37 ` Cyril Bur
0 siblings, 2 replies; 6+ messages in thread
From: Jeremy Kerr @ 2016-05-26 2:25 UTC (permalink / raw)
To: OpenBMC Patches, openbmc; +Cc: Nan Li
Hi Nan,
You seem to have a minor typo in the commit title there.
> - // TODO: Issue 5: This is not endian-safe.
> - short *recid = (short*) &reqptr->selrecordls;
> - short *offset = (short*) &reqptr->offsetls;
> +
> + unsigned short recid = ((unsigned short) reqptr->selrecordms) << 8 + reqptr->selrecordls;
> + unsigned short offset = ((unsigned short) reqptr->offsetms) << 8 + reqptr->offsetls;
That won't do the endian conversion correctly. Can you use one of the
existing endian conversion functions?
The le16toh() function is probably what you want here, from endian.h.
Also, be careful of the change from 'short' to 'unsigned short'.
Regards,
Jeremy
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
2016-05-26 2:25 ` Jeremy Kerr
@ 2016-05-26 3:17 ` Nan KX Li
2016-05-26 3:37 ` Cyril Bur
1 sibling, 0 replies; 6+ messages in thread
From: Nan KX Li @ 2016-05-26 3:17 UTC (permalink / raw)
To: Jeremy Kerr; +Cc: openbmc, OpenBMC Patches
[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]
Hi Jeremy,
Thanks.
(I missed Cyril's mail.)
I'd use the conversion function to change my commit.
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@ozlabs.org>
To: OpenBMC Patches <openbmc-patches@stwcx.xyz>,
openbmc@lists.ozlabs.org
Cc: Nan KX Li/China/IBM@IBMCN
Date: 05/26/2016 10:27
Subject: Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness
issue5
Hi Nan,
You seem to have a minor typo in the commit title there.
> - // TODO: Issue 5: This is not endian-safe.
> - short *recid = (short*) &reqptr->selrecordls;
> - short *offset = (short*) &reqptr->offsetls;
> +
> + unsigned short recid = ((unsigned short)
reqptr->selrecordms) << 8 + reqptr->selrecordls;
> + unsigned short offset = ((unsigned short)
reqptr->offsetms) << 8 + reqptr->offsetls;
That won't do the endian conversion correctly. Can you use one of the
existing endian conversion functions?
The le16toh() function is probably what you want here, from endian.h.
Also, be careful of the change from 'short' to 'unsigned short'.
Regards,
Jeremy
[-- Attachment #2: Type: text/html, Size: 2863 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
2016-05-26 2:25 ` Jeremy Kerr
2016-05-26 3:17 ` Nan KX Li
@ 2016-05-26 3:37 ` Cyril Bur
2016-05-26 6:19 ` Nan KX Li
1 sibling, 1 reply; 6+ messages in thread
From: Cyril Bur @ 2016-05-26 3:37 UTC (permalink / raw)
To: Nan Li; +Cc: Jeremy Kerr, openbmc
On Thu, 26 May 2016 10:25:34 +0800
Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Nan,
>
Hi Nan,
I responded to v1 of your patch. You have sent exactly the same code as v2.
One of two things has happened:
One you accidentally triggered github to send a v2 (which admittedly is easy to
do), please try to be more careful in future, this creates a lot of pointless
noise on the list and leads to double reviews, Jeremy has made the same point
I previously have.
Two you're ignoring reviews to your patches and sending subsequent versions...
why do you do this?
> You seem to have a minor typo in the commit title there.
>
> > - // TODO: Issue 5: This is not endian-safe.
> > - short *recid = (short*) &reqptr->selrecordls;
> > - short *offset = (short*) &reqptr->offsetls;
> > +
> > + unsigned short recid = ((unsigned short) reqptr->selrecordms) << 8 + reqptr->selrecordls;
> > + unsigned short offset = ((unsigned short) reqptr->offsetms) << 8 + reqptr->offsetls;
>
> That won't do the endian conversion correctly. Can you use one of the
> existing endian conversion functions?
>
> The le16toh() function is probably what you want here, from endian.h.
>
> Also, be careful of the change from 'short' to 'unsigned short'.
>
> Regards,
>
>
> Jeremy
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5
2016-05-26 3:37 ` Cyril Bur
@ 2016-05-26 6:19 ` Nan KX Li
0 siblings, 0 replies; 6+ messages in thread
From: Nan KX Li @ 2016-05-26 6:19 UTC (permalink / raw)
To: Cyril Bur; +Cc: Jeremy Kerr, openbmc
[-- Attachment #1: Type: text/plain, Size: 2133 bytes --]
Hi Cyril and Jeremy,
I'm sorry for that.
This morning I changed my code and accidentally pushed to github before I
found your reviews. I will be more careful in future.
Regards,
William Li ( Li Nan, 李楠 )
Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605
Beijing, China
From: Cyril Bur <cyrilbur@gmail.com>
To: Nan KX Li/China/IBM@IBMCN
Cc: Jeremy Kerr <jk@ozlabs.org>, openbmc@lists.ozlabs.org
Date: 05/26/2016 12:04
Subject: Re: [PATCH openpower-host-ipmi-oem v2] Fix endianness
issue5
On Thu, 26 May 2016 10:25:34 +0800
Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Nan,
>
Hi Nan,
I responded to v1 of your patch. You have sent exactly the same code as
v2.
One of two things has happened:
One you accidentally triggered github to send a v2 (which admittedly is
easy to
do), please try to be more careful in future, this creates a lot of
pointless
noise on the list and leads to double reviews, Jeremy has made the same
point
I previously have.
Two you're ignoring reviews to your patches and sending subsequent
versions...
why do you do this?
> You seem to have a minor typo in the commit title there.
>
> > - // TODO: Issue 5: This is not endian-safe.
> > - short *recid = (short*) &reqptr->selrecordls;
> > - short *offset = (short*) &reqptr->offsetls;
> > +
> > + unsigned short recid = ((unsigned short)
reqptr->selrecordms) << 8 + reqptr->selrecordls;
> > + unsigned short offset = ((unsigned short)
reqptr->offsetms) << 8 + reqptr->offsetls;
>
> That won't do the endian conversion correctly. Can you use one of the
> existing endian conversion functions?
>
> The le16toh() function is probably what you want here, from endian.h.
>
> Also, be careful of the change from 'short' to 'unsigned short'.
>
> Regards,
>
>
> Jeremy
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc
[-- Attachment #2: Type: text/html, Size: 3978 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-26 6:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-26 2:00 [PATCH openpower-host-ipmi-oem v2] Fix endianness issue5 OpenBMC Patches
2016-05-26 2:00 ` OpenBMC Patches
2016-05-26 2:25 ` Jeremy Kerr
2016-05-26 3:17 ` Nan KX Li
2016-05-26 3:37 ` Cyril Bur
2016-05-26 6:19 ` Nan KX Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.