[PATCH phosphor-host-ipmid] ipmi daemon return code modification

[Patrick Williams]

On Wed, Mar 09, 2016 at 06:56:17PM +0000, Chris Austen wrote:
> Patrick is there work needed to fix something here?   If so open an issue 
>    

Opened https://github.com/openbmc/phosphor-host-ipmid/issues/80


>     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.  
>     
>     
>      
>        Sent from my iPhone using IBM Verse
>    
>    On Mar 9, 2016, 9:09:21 AM, patrick at stwcx.xyz wrote:
>    
>    From: patrick at stwcx.xyz
>    To: cyrilbur at gmail.com
>    Cc: openbmc at lists.ozlabs.org, tomjoseph at in.ibm.com, openbmc-patches at stwcx.xyz
>    Date: Mar 9, 2016 9:09:21 AM
>    Subject: Re: [PATCH phosphor-host-ipmid] ipmi daemon return code modification
>    
>    
>        I agree with Cyril's comments.  This code should not have been merged in
>     the current state.
>     The first line directly contradicts the comments.  Need to update the
>     comment to explain why '8' is needed now.  And removing an error path is
>     non-obvious.  Even the commit message doesn't really give any details on
>     what / why this change was made.
>     On Wed, Mar 09, 2016 at 01:31:23PM +1100, Cyril Bur wrote:
>     > On Tue,  8 Mar 2016 14:10:33 -0600
>     > OpenBMC Patches  wrote:
>     > 
>     > > From: tomjose 
>     > > 
>     > > ---
>     > >  apphandler.C | 2 +-
>     > >  ipmid.C      | 1 -
>     > >  2 files changed, 1 insertion(+), 2 deletions(-)
>     > > 
>     > > diff --git a/apphandler.C b/apphandler.C
>     > > index a921643..fc6c811 100644
>     > > --- a/apphandler.C
>     > > +++ b/apphandler.C
>     > > @@ -358,7 +358,7 @@ ipmi_ret_t ipmi_app_channel_info(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>     > >      printf("IPMI APP GET CHANNEL INFO\n");
>     > >  
>     > >      // I"m only supporting channel 1.  0xE is the 'default channel'
>     > > -    if (*p == 0xe || *p == 1) {
>     > > +    if (*p == 0xe || *p == 1 || *p == 8) {
>     > >  
>     > 
>     > So I take it that the comment is no longer valid? Can we take a break on magic
>     > numbers or at least quote and link the spec nearby?
>     > 
>     > >          *data_len = sizeof(resp);
>     > >          memcpy(response, resp, *data_len);
>     > > diff --git a/ipmid.C b/ipmid.C
>     > > index 6726a27..728ba0b 100644
>     > > --- a/ipmid.C
>     > > +++ b/ipmid.C
>     > > @@ -267,7 +267,6 @@ static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
>     > >      if(r != 0)
>     > >      {
>     > >          fprintf(stderr,"ERROR:[0x%X] handling NetFn:[0x%X], Cmd:[0x%X]\n",r, netfn, cmd);
>     > > -        return -1;
>     > >      }
>     > 
>     > Was this change intended?
>     > 
>     > >  
>     > >      fprintf(ipmiio, "IPMI Response:\n");
>     > 
>     > _______________________________________________
>     > openbmc mailing list
>     > openbmc at lists.ozlabs.org
>     > https://lists.ozlabs.org/listinfo/openbmc
>     -- 
>     Patrick Williams
>     _______________________________________________
>     openbmc mailing list
>     openbmc at lists.ozlabs.org
>     https://lists.ozlabs.org/listinfo/openbmc

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160318/c2a6313d/attachment.sig>