<html><head><META http-equiv="Content-Type" content="text/html;charset=utf-8"></head><body><div id="content" contenteditable="true" spellcheck="true" autocorrect="true" autocapitalize="true" style="font-family: Helvetica;">Patrick is there work needed to fix something here? If so open an issue <div><br></div><div>I think the lesson here is we need more integration testing (aka warm hand offs) with Foxconn. I chose channel 1 without thinking too much about it. Turns out they use channel 8. </div><div><br></div><div><br><div><br></div><div><br>Sent from my iPhone using IBM Verse<br><br><hr>On Mar 9, 2016, 9:09:21 AM, patrick@stwcx.xyz wrote:<br><br>From: patrick@stwcx.xyz<br>To: cyrilbur@gmail.com<br>Cc: openbmc@lists.ozlabs.org, tomjoseph@in.ibm.com, openbmc-patches@stwcx.xyz<br>Date: Mar 9, 2016 9:09:21 AM<br>Subject: Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification<br><br><div id="MaaS360PIMSDKOriginalMessageId">I agree with Cyril's comments. This code should not have been merged in<br>the current state.<br>The first line directly contradicts the comments. Need to update the<br>comment to explain why '8' is needed now. And removing an error path is<br>non-obvious. Even the commit message doesn't really give any details on<br>what / why this change was made.<br>On Wed, Mar 09, 2016 at 01:31:23PM +1100, Cyril Bur wrote:<br>> On Tue, 8 Mar 2016 14:10:33 -0600<br>> OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br>> <br>> > From: tomjose <tomjoseph@in.ibm.com><br>> > <br>> > ---<br>> > apphandler.C | 2 +-<br>> > ipmid.C | 1 -<br>> > 2 files changed, 1 insertion(+), 2 deletions(-)<br>> > <br>> > diff --git a/apphandler.C b/apphandler.C<br>> > index a921643..fc6c811 100644<br>> > --- a/apphandler.C<br>> > +++ b/apphandler.C<br>> > @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> > printf("IPMI APP GET CHANNEL INFO\n");<br>> > <br>> > // I"m only supporting channel 1. 0xE is the 'default channel'<br>> > - if (*p == 0xe || *p == 1) {<br>> > + if (*p == 0xe || *p == 1 || *p == 8) {<br>> > <br>> <br>> So I take it that the comment is no longer valid? Can we take a break on magic<br>> numbers or at least quote and link the spec nearby?<br>> <br>> > *data_len = sizeof(resp);<br>> > memcpy(response, resp, *data_len);<br>> > diff --git a/ipmid.C b/ipmid.C<br>> > index 6726a27..728ba0b 100644<br>> > --- a/ipmid.C<br>> > +++ b/ipmid.C<br>> > @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error<br>> > if(r != 0)<br>> > {<br>> > fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);<br>> > - return -1;<br>> > }<br>> <br>> Was this change intended?<br>> <br>> > <br>> > fprintf(ipmiio, "IPMI Response:\n");<br>> <br>> _______________________________________________<br>> openbmc mailing list<br>> openbmc@lists.ozlabs.org<br>> https://lists.ozlabs.org/listinfo/openbmc<br>-- <br>Patrick Williams<br>_______________________________________________<br>openbmc mailing list<br>openbmc@lists.ozlabs.org<br>https://lists.ozlabs.org/listinfo/openbmc<br></tomjoseph@in.ibm.com></openbmc-patches@stwcx.xyz></div></div></div></div><BR>
</body></html>