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

[Andrew Jeffery]

Hi,

On Mon, 2016-05-16 at 22:10 +0800, Nan KX Li wrote:
> 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.

Yeah, I agree. This is the kind of thing I was referring to as in my
opening paragraph - it'd be nice to clean it up but I wouldn't bother
with that here. I think returning 0 should be enough.

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160517/50b27189/attachment-0001.sig>