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

[Chris Austen]

Patrick is there work needed to fix something here?   If so open an issue 
   
    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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160309/07911e29/attachment.html>