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

[Patrick Williams]

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 <openbmc-patches at stwcx.xyz> wrote:
> 
> > From: tomjose <tomjoseph at in.ibm.com>
> > 
> > ---
> >  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
-------------- 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/20160309/38ca410a/attachment.sig>