[PATCH phosphor-host-ipmid v2] Handle errors finding openbmc_path #75

[Nan KX Li]

Thanks Andrew. 
I think you are right. I'll improve my code accordingly. Also I give some 
comments below.


Regards,

William Li ( Li Nan, 李楠 ) 
 

Firmware Engineering Professional
OpenPower AE Team | IBM System & Technology Lab
Mobile: +86-186-1081 6605

Beijing, China



From:   Andrew Jeffery <andrew at aj.id.au>
To:     OpenBMC Patches <openbmc-patches at stwcx.xyz>, 
openbmc at lists.ozlabs.org
Cc:     Nan KX Li/China/IBM at IBMCN
Date:   05/16/2016 10:27
Subject:        Re: [PATCH phosphor-host-ipmid v2] Handle errors finding 
openbmc_path #75



> Hi Nan Li,
> 
> Thanks for the patch. I've had a look at the files you're modifying and
> it seems like the error handling is ripe for improvement beyond the
> issues you've fixed. However, resolving those problems is probably
> beyond the scope of your patch, so never mind that.
> 
> On Fri, 2016-05-13 at 07:00 -0500, OpenBMC Patches wrote:
> > From: Nan Li <bjlinan at cn.ibm.com>
> > 
> > 1.Check return value properly.
> > 
> > Signed-off-by: Nan Li <bjlinan at cn.ibm.com>
> > ---
> >  ipmid.C         | 10 ++++++++++
> >  sensorhandler.C |  9 ++++++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ipmid.C b/ipmid.C
> > index 728ba0b..413c2b7 100644
> > --- a/ipmid.C
> > +++ b/ipmid.C
> > @@ -565,6 +565,11 @@ int set_sensor_dbus_state_s(uint8_t number, 
> const char *method, const char *valu
> >  
> >      r = find_openbmc_path("SENSOR", number, &a);
> >  
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to find Sensor 0x%02x\n", number);
> > +        goto final;
> > +    }
> > +
> 
> We jump to the final label which is consistent with the existing error
> handling, but we don't appear to need any of the clean-up that's done
> there. Maybe we should just return instead?
> 
> If we return, we need to return a value. Looking at the existing code
> it seems that 0 is returned regardless of whether an error has
> occurred. This doesn't seem useful (i.e. should this just be a void
> function?), but also means that you don't need to choose an error
> value...
> 

I think that changing it to a void function will lead to more code change, 
like struct lookup_t[]. 
I'm not sure if those change are acceptable. so I prefer to keep things as 
they are.

> 
> >      r = sd_bus_message_new_method_call
> (bus,&m,a.bus,a.path,a.interface,method);
> >      if (r < 0) {
> >          fprintf(stderr, "Failed to create a method call: %s" 
> strerror(-r));
> > @@ -602,6 +607,11 @@ int set_sensor_dbus_state_y(uint8_t number, 
> const char *method, const uint8_t va
> >  
> >      r = find_openbmc_path("SENSOR", number, &a);
> >  
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to find Sensor 0x%02x\n", number);
> > +        goto final;
> > +    }
> > +
> >      r = sd_bus_message_new_method_call
> (bus,&m,a.bus,a.path,a.interface,method);
> >      if (r < 0) {
> >          fprintf(stderr, "Failed to create a method call: %s", 
> strerror(-r));
> > diff --git a/sensorhandler.C b/sensorhandler.C
> > index bb14e7a..9c7c2cd 100644
> > --- a/sensorhandler.C
> > +++ b/sensorhandler.C
> > @@ -180,6 +180,13 @@ ipmi_ret_t ipmi_sen_get_sensor_reading
> (ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> >  
> >      r = find_openbmc_path("SENSOR", reqptr->sennum, &a);
> >  
> > +    if (r < 0) {
> > +        fprintf(stderr, "Failed to find Sensor 0x%02x\n", 
reqptr->sennum);
> > +        *data_len=0;
> > +        rc = IPMI_CC_SENSOR_INVALID;
> > +        goto final;
> 
> Instead of 'goto final' we should just 'return IPMI_CC_SENSOR_INVALID'.
> If we do that we also don't need to introduce the label.
> 
> > +    }
> > +
> >      type = find_sensor(reqptr->sennum);
> >  
> >      fprintf(stderr, "Bus: %s, Path: %s, Interface: %s\n", a.bus, 
> a.path, a.interface);
> > @@ -220,7 +227,7 @@ ipmi_ret_t ipmi_sen_get_sensor_reading
> (ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> >              break;
> >      }
> >  
> > -
> > +final:
> >      reply = sd_bus_message_unref(reply);
> 
> Since we're not using reply at the point we've introduced the goto, we
> don't need to unref it. We know it'll still be NULL.
> sd_bus_message_unref() handles the NULL case, but this is something I
> had to dive into the code to verify. For sanity's sake we should
> probably avoid it.

> Cheers,
> 
> Andrew
> 
> >  
> >      return rc;[attachment "signature.asc" deleted by Nan KX 
Li/China/IBM] 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160516/5029cb80/attachment.html>