* [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.