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

[Andrew Jeffery]

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...


>      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;
-------------- 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/20160516/b32572b2/attachment.sig>