[PATCH phosphor-host-ipmid] Alignment and double error

Joel Stanley joel at jms.id.au
Thu Dec 3 11:56:58 AEDT 2015


On Thu, Dec 3, 2015 at 10:50 AM, OpenBMC Patches
<openbmc-patches at stwcx.xyz> wrote:
> From: Chris Austen <austenc at us.ibm.com>
>
> Fixed a pointer pointer issue where no one had the storage for a uint16_t
> Fixed the double error log on palmetto systems needed a reconfig loop

These unrelated changes should be two separate commits.

> ---
>  ipmid.C         | 19 ++++++++++---------
>  sensorhandler.C | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  sensorhandler.h |  1 +
>  storageaddsel.C | 30 ++++++------------------------
>  4 files changed, 65 insertions(+), 33 deletions(-)
>
> diff --git a/ipmid.C b/ipmid.C
> index f1f938c..5b17036 100644
> --- a/ipmid.C
> +++ b/ipmid.C
> @@ -186,7 +186,7 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch
>
>      r = sd_bus_message_new_method_call(bus,&m,dest,path,DBUS_INTF,"sendMessage");
>      if (r < 0) {
> -        fprintf(stderr, "Failed to add the method object: %s\n", strerror(-r));
> +        fprintf(stderr, "Failed to add the method object: %s %s\n", __FUNCTION__, strerror(-r));
>          return -1;
>      }
>
> @@ -198,14 +198,14 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch
>      // Add the bytes needed for the methods to be called
>      r = sd_bus_message_append(m, "yyyyy", seq, netfn, lun, cmd, cc);
>      if (r < 0) {
> -        fprintf(stderr, "Failed add the netfn and others : %s\n", strerror(-r));
> -        return -1;
> +        fprintf(stderr, "Failed add the netfn and others : %s %s\n", __FUNCTION__, strerror(-r));
> +        goto final;
>      }
>
>      r = sd_bus_message_append_array(m, 'y', buf, len);
>      if (r < 0) {
> -        fprintf(stderr, "Failed to add the string of response bytes: %s\n", strerror(-r));
> -        return -1;
> +        fprintf(stderr, "Failed to add the string of response bytes: %s %s\n", __FUNCTION__, strerror(-r));
> +        goto final;
>      }
>
>
> @@ -213,18 +213,19 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch
>      // Call the IPMI responder on the bus so the message can be sent to the CEC
>      r = sd_bus_call(bus, m, 0, &error, &reply);
>      if (r < 0) {
> -        fprintf(stderr, "Failed to call the method: %s", strerror(-r));
> -        return -1;
> +        fprintf(stderr, "Failed to call the method: %s %s\n", __FUNCTION__, strerror(-r));
> +        goto final;
>      }
>
>      r = sd_bus_message_read(reply, "x", &pty);
>      if (r < 0) {
> -       fprintf(stderr, "Failed to get a rc from the method: %s\n", strerror(-r));
> -
> +       fprintf(stderr, "Failed to get a rc from the method: %s %s\n", __FUNCTION__, strerror(-r));
>      }
>
> +final:
>      sd_bus_error_free(&error);
>      sd_bus_message_unref(m);
> +    sd_bus_message_unref(reply);
>
>
>      return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;
> diff --git a/sensorhandler.C b/sensorhandler.C
> index dd83152..131da71 100644
> --- a/sensorhandler.C
> +++ b/sensorhandler.C
> @@ -29,6 +29,7 @@ sensorTypemap_t g_SensorTypeMap[] = {
>      {0xe9, 0x09, "OccStatus"},  // E9 is an internal mapping to handle sensor type code os 0x09
>      {0xC3, 0x6F, "BootCount"},
>      {0x1F, 0x6F, "OperatingSystemStatus"},
> +    {0x12, 0x6F, "SYSTEM"},
>      {0xFF, 0x00, ""},
>  };
>
> @@ -39,6 +40,14 @@ struct sensor_data_t {
>  }  __attribute__ ((packed)) ;
>
>
> +struct sensor_reading_ret_t {
> +    uint8_t reading;
> +    uint8_t options;
> +    uint8_t assert[2];
> +}  __attribute__ ((packed)) ;
> +
> +
> +

Whitespace

>  uint8_t dbus_to_sensor_type(char *p) {
>
>      sensorTypemap_t *s = g_SensorTypeMap;
> @@ -155,6 +164,42 @@ ipmi_ret_t ipmi_sen_set_sensor(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>      return rc;
>  }
>
> +

Whitespace

> +ipmi_ret_t ipmi_sen_get_sensor_reading(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> +                             ipmi_request_t request, ipmi_response_t response,
> +                             ipmi_data_len_t data_len, ipmi_context_t context)
> +{
> +    sensor_data_t *reqptr = (sensor_data_t*)request;
> +    sensor_reading_ret_t ret;
> +    ipmi_ret_t rc;
> +
> +    printf("IPMI GET_SENSOR_READING [0x%02x]\n",reqptr->sennum);
> +
> +

Whitespace

> +    // TODO Call dbus method to get teh real state of the sensor and
> +    // translate it to IPMI.  For now stop every boot of a Palmetto
> +    // from creating error logs about missing the Reboot Sensor.
> +    if (reqptr->sennum == 9) {

Give the magic number a name.

> +        ret.reading = 2;

Again, what does 2 mean?

> +        ret.options = 0;
> +        ret.assert[0] = 0;
> +        ret.assert[1] = 0;
> +
> +        *data_len = sizeof(ret);
> +        memcpy(response, &ret, *data_len);

Is there a reason why you create the local ret, and then copy the
results in to response? Could you just put the data straight into
response?

> +        rc = IPMI_CC_OK;
> +
> +    } else {
> +        *data_len = 0;
> +        rc = IPMI_CC_SENSOR_INVALID;
> +    }
> +
> +

Whitespace

> +    return rc;
> +}
> +
> +
> +

Whitespace.

>  ipmi_ret_t ipmi_sen_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>                               ipmi_request_t request, ipmi_response_t response,
>                               ipmi_data_len_t data_len, ipmi_context_t context)
> @@ -179,5 +224,8 @@ void register_netfn_sen_functions()
>      printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_SENSOR, IPMI_CMD_SET_SENSOR);
>      ipmi_register_callback(NETFUN_SENSOR, IPMI_CMD_SET_SENSOR, NULL, ipmi_sen_set_sensor);
>
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_SENSOR, IPMI_CMD_GET_SENSOR_READING);
> +    ipmi_register_callback(NETFUN_SENSOR, IPMI_CMD_GET_SENSOR_READING, NULL, ipmi_sen_get_sensor_reading);
> +
>      return;

This is redundant.

>  }
> diff --git a/sensorhandler.h b/sensorhandler.h
> index 7b89a18..dd940dc 100644
> --- a/sensorhandler.h
> +++ b/sensorhandler.h
> @@ -6,6 +6,7 @@
>  // IPMI commands for net functions.
>  enum ipmi_netfn_sen_cmds
>  {
> +    IPMI_CMD_GET_SENSOR_READING = 0x2D,
>      IPMI_CMD_GET_SENSOR_TYPE = 0x2F,
>      IPMI_CMD_SET_SENSOR      = 0x30,
>  };
> diff --git a/storageaddsel.C b/storageaddsel.C
> index 345dfd5..991c78d 100644
> --- a/storageaddsel.C
> +++ b/storageaddsel.C
> @@ -155,7 +155,7 @@ int send_esel_to_dbus(const char *desc, const char *sev, const char *details, ui
>         sd_bus *mbus = NULL;
>      sd_bus_error error = SD_BUS_ERROR_NULL;
>      sd_bus_message *reply = NULL, *m=NULL;
> -    uint16_t *pty;
> +    uint16_t x;

Use a more descriptive name than "x".

>      int r;
>
>      mbus = ipmid_get_sd_bus_connection();
> @@ -181,26 +181,22 @@ int send_esel_to_dbus(const char *desc, const char *sev, const char *details, ui
>          fprintf(stderr, "Failed to add the raw array of bytes: %s\n", strerror(-r));
>          return -1;
>      }
> -
>      // Call the IPMI responder on the bus so the message can be sent to the CEC
>      r = sd_bus_call(mbus, m, 0, &error, &reply);
>      if (r < 0) {
> -        fprintf(stderr, "Failed to call the method: %s", strerror(-r));
> +        fprintf(stderr, "Failed to call the method: %s %s\n", __FUNCTION__, strerror(-r));

Systemd gives us a string in error->message. You could print that
instead of strerror.

>          return -1;
>      }
> -
> -    r = sd_bus_message_read(reply, "q", &pty);
> +    r = sd_bus_message_read(reply, "q", &x);
>      if (r < 0) {
>          fprintf(stderr, "Failed to get a rc from the method: %s\n", strerror(-r));
> -    } else {
> -        r = *pty;
>      }
>
> +finish:

storageaddsel.C:195:1: warning: label ‘finish’ defined but not used
[-Wunused-label]
 finish:
 ^

>      sd_bus_error_free(&error);
>      sd_bus_message_unref(m);
>      sd_bus_message_unref(reply);
> -
> -    return r;
> +    return 0;

This function will print an error message when sd_bus_message_read
fails, but will always return zero now. Is that the intention?

>  }
>
>
> @@ -213,41 +209,27 @@ void send_esel(uint16_t recordid) {
>         int r;
>
>         uint8_t hack[] = {0x30, 0x32, 0x34};
> -
>         asprintf(&path,"%s%04x", "/tmp/esel", recordid);
> -
>         sz = getfilestream(path, &buffer);
> -
>         if (sz == 0) {
>                 printf("Error file does not exist %d\n",__LINE__);
>                 free(path);
>                 return;
>         }
>
> -
>         sev = create_esel_severity(buffer);
> -
>         create_esel_association(buffer, &assoc);
> -
>         create_esel_description(buffer, sev, &desc);
>
> -
>         // TODO until ISSUE https://github.com/openbmc/rest-dbus/issues/2
>         // I cant send extended ascii chars.  So 0,2,4 for now...
> -       r = send_esel_to_dbus(desc, sev, assoc, hack, 3);
> -
> -       asprintf(&pathsent,"%s_%d", path, r);
> -
> -
> -       rename(path, pathsent);

storageaddsel.C: In function ‘void send_esel(uint16_t)’:
storageaddsel.C:209:6: warning: unused variable ‘r’ [-Wunused-variable]
  int r;
      ^
storageaddsel.C:229:16: warning: ‘pathsent’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
  free(pathsent);
                ^
storageaddsel.C:207:15: note: ‘pathsent’ was declared here
  char *path, *pathsent;
               ^

> +       send_esel_to_dbus(desc, sev, assoc, hack, 3);
>
>         free(path);
>         free(pathsent);
>         free(assoc);
>         free(desc);
> -
>         delete[] buffer;
>
> -
>         return;
>  }
> --
> 2.6.3
>
>
> _______________________________________________
> openbmc mailing list
> openbmc at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc


More information about the openbmc mailing list