<font size=2 face="sans-serif">Thanks Andrew. </font><br><font size=2 face="sans-serif">I think you are right. I'll improve
my code accordingly. Also I give some comments below.</font><br><br><br><font size=3 color=#424282 face="Calibri">Regards,</font><br><br><font size=3 color=#424282 face="Calibri">William Li ( Li Nan, Àîéª
) </font><br><font size=3 color=#424282 face="Calibri">   </font><br><br><font size=3 color=#424282 face="Calibri">Firmware Engineering Professional</font><br><font size=3 color=#424282 face="Calibri">OpenPower AE Team | IBM System
& Technology Lab</font><br><font size=3 color=#424282 face="Calibri">Mobile: +86-186-1081 6605</font><br><br><font size=3 color=#424282 face="Calibri">Beijing, China</font><br><br><br><br><font size=1 color=#5f5f5f face="sans-serif">From:      
 </font><font size=1 face="sans-serif">Andrew Jeffery <andrew@aj.id.au></font><br><font size=1 color=#5f5f5f face="sans-serif">To:      
 </font><font size=1 face="sans-serif">OpenBMC Patches <openbmc-patches@stwcx.xyz>,
openbmc@lists.ozlabs.org</font><br><font size=1 color=#5f5f5f face="sans-serif">Cc:      
 </font><font size=1 face="sans-serif">Nan KX Li/China/IBM@IBMCN</font><br><font size=1 color=#5f5f5f face="sans-serif">Date:      
 </font><font size=1 face="sans-serif">05/16/2016 10:27</font><br><font size=1 color=#5f5f5f face="sans-serif">Subject:    
   </font><font size=1 face="sans-serif">Re: [PATCH phosphor-host-ipmid
v2] Handle errors finding openbmc_path #75</font><br><hr noshade><br><br><br><tt><font size=2>> Hi Nan Li,<br>> <br>> Thanks for the patch. I've had a look at the files you're modifying
and<br>> it seems like the error handling is ripe for improvement beyond the<br>> issues you've fixed. However, resolving those problems is probably<br>> beyond the scope of your patch, so never mind that.<br>> <br>> On Fri, 2016-05-13 at 07:00 -0500, OpenBMC Patches wrote:<br>> > From: Nan Li <bjlinan@cn.ibm.com><br>> > <br>> > 1.Check return value properly.<br>> > <br>> > Signed-off-by: Nan Li <bjlinan@cn.ibm.com><br>> > ---<br>> >  ipmid.C         |
10 ++++++++++<br>> >  sensorhandler.C |  9 ++++++++-<br>> >  2 files changed, 18 insertions(+), 1 deletion(-)<br>> > <br>> > diff --git a/ipmid.C b/ipmid.C<br>> > index 728ba0b..413c2b7 100644<br>> > --- a/ipmid.C<br>> > +++ b/ipmid.C<br>> > @@ -565,6 +565,11 @@ int set_sensor_dbus_state_s(uint8_t number,
<br>> const char *method, const char *valu<br>> >  <br>> >      r = find_openbmc_path("SENSOR",
number, &a);<br>> >  <br>> > +    if (r < 0) {<br>> > +        fprintf(stderr,
"Failed to find Sensor 0x%02x\n", number);<br>> > +        goto final;<br>> > +    }<br>> > +<br>> <br>> We jump to the final label which is consistent with the existing error<br>> handling, but we don't appear to need any of the clean-up that's done<br>> there. Maybe we should just return instead?<br>> <br>> If we return, we need to return a value. Looking at the existing code<br>> it seems that 0 is returned regardless of whether an error has<br>> occurred. This doesn't seem useful (i.e. should this just be a void<br>> function?), but also means that you don't need to choose an error<br>> value...<br>> </font></tt><br><br><tt><font size=2>I think that changing it to a void function will lead
to more code change, like struct lookup_t[]. </font></tt><br><tt><font size=2>I'm not sure if those change are acceptable. so I
prefer to keep things as they are.</font></tt><br><tt><font size=2><br>> <br>> >      r = sd_bus_message_new_method_call<br>> (bus,&m,a.bus,a.path,a.interface,method);<br>> >      if (r < 0) {<br>> >          fprintf(stderr,
"Failed to create a method call: %s" <br>> strerror(-r));<br>> > @@ -602,6 +607,11 @@ int set_sensor_dbus_state_y(uint8_t number,
<br>> const char *method, const uint8_t va<br>> >  <br>> >      r = find_openbmc_path("SENSOR",
number, &a);<br>> >  <br>> > +    if (r < 0) {<br>> > +        fprintf(stderr,
"Failed to find Sensor 0x%02x\n", number);<br>> > +        goto final;<br>> > +    }<br>> > +<br>> >      r = sd_bus_message_new_method_call<br>> (bus,&m,a.bus,a.path,a.interface,method);<br>> >      if (r < 0) {<br>> >          fprintf(stderr,
"Failed to create a method call: %s", <br>> strerror(-r));<br>> > diff --git a/sensorhandler.C b/sensorhandler.C<br>> > index bb14e7a..9c7c2cd 100644<br>> > --- a/sensorhandler.C<br>> > +++ b/sensorhandler.C<br>> > @@ -180,6 +180,13 @@ ipmi_ret_t ipmi_sen_get_sensor_reading<br>> (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> >  <br>> >      r = find_openbmc_path("SENSOR",
reqptr->sennum, &a);<br>> >  <br>> > +    if (r < 0) {<br>> > +        fprintf(stderr,
"Failed to find Sensor 0x%02x\n", reqptr->sennum);<br>> > +        *data_len=0;<br>> > +        rc = IPMI_CC_SENSOR_INVALID;<br>> > +        goto final;<br>> <br>> Instead of 'goto final' we should just 'return IPMI_CC_SENSOR_INVALID'.<br>> If we do that we also don't need to introduce the label.<br>> <br>> > +    }<br>> > +<br>> >      type = find_sensor(reqptr->sennum);<br>> >  <br>> >      fprintf(stderr, "Bus: %s,
Path: %s, Interface: %s\n", a.bus, <br>> a.path, a.interface);<br>> > @@ -220,7 +227,7 @@ ipmi_ret_t ipmi_sen_get_sensor_reading<br>> (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> >              break;<br>> >      }<br>> >  <br>> > -<br>> > +final:<br>> >      reply = sd_bus_message_unref(reply);<br>> <br>> Since we're not using reply at the point we've introduced the goto,
we<br>> don't need to unref it. We know it'll still be NULL.<br>> sd_bus_message_unref() handles the NULL case, but this is something
I<br>> had to dive into the code to verify. For sanity's sake we should<br>> probably avoid it.</font></tt><br><tt><font size=2><br>> Cheers,<br>> <br>> Andrew<br>> <br>> >  <br>> >      return rc;[attachment "signature.asc"
deleted by Nan KX Li/China/IBM] </font></tt><br><BR>