<font face="Default Sans Serif,Verdana,Arial,Helvetica,sans-serif" size="2">I removed the sensor reading feature.  I'll use a separate commit for that<span> in the next sprint. <br><br><br>Chris Austen<br>POWER Systems Enablement Manager <br>(512) 286-5184 (T/L: 363-5184)</span><br><br><font color="#990099">-----"openbmc" <<a target="_blank" href="mailto:openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org">openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org</a>> wrote: -----</font><div class="iNotesHistory" style="padding-left:5px;"><div style="padding-right:0px;padding-left:5px;border-left:solid black 2px;">To: OpenBMC Patches <<a target="_blank" href="mailto:openbmc-patches@stwcx.xyz">openbmc-patches@stwcx.xyz</a>><br>From: Joel Stanley <joel@jms.id.au><br>Sent by: "openbmc" <openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org><br>Date: 12/02/2015 06:57PM<br>Cc: OpenBMC Maillist <<a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a>><br>Subject: Re: [PATCH phosphor-host-ipmid] Alignment and double error<br><br><div><font size="2" face="Courier New,Courier,monospace">On Thu, Dec 3, 2015 at 10:50 AM, OpenBMC Patches<br><<a target="_blank" href="mailto:openbmc-patches@stwcx.xyz">openbmc-patches@stwcx.xyz</a>> wrote:<br>> From: Chris Austen <<a target="_blank" href="mailto:austenc@us.ibm.com">austenc@us.ibm.com</a>><br>><br>> Fixed a pointer pointer issue where no one had the storage for a uint16_t<br>> Fixed the double error log on palmetto systems needed a reconfig loop<br><br>These unrelated changes should be two separate commits.<br><br>> ---<br>>  ipmid.C         | 19 ++++++++++---------<br>>  sensorhandler.C | 48 ++++++++++++++++++++++++++++++++++++++++++++++++<br>>  sensorhandler.h |  1 +<br>>  storageaddsel.C | 30 ++++++------------------------<br>>  4 files changed, 65 insertions(+), 33 deletions(-)<br>><br>> diff --git a/ipmid.C b/ipmid.C<br>> index f1f938c..5b17036 100644<br>> --- a/ipmid.C<br>> +++ b/ipmid.C<br>> @@ -186,7 +186,7 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch<br>><br>>      r = sd_bus_message_new_method_call(bus,&m,dest,path,DBUS_INTF,"sendMessage");<br>>      if (r < 0) {<br>> -        fprintf(stderr, "Failed to add the method object: %s\n", strerror(-r));<br>> +        fprintf(stderr, "Failed to add the method object: %s %s\n", __FUNCTION__, strerror(-r));<br>>          return -1;<br>>      }<br>><br>> @@ -198,14 +198,14 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch<br>>      // Add the bytes needed for the methods to be called<br>>      r = sd_bus_message_append(m, "yyyyy", seq, netfn, lun, cmd, cc);<br>>      if (r < 0) {<br>> -        fprintf(stderr, "Failed add the netfn and others : %s\n", strerror(-r));<br>> -        return -1;<br>> +        fprintf(stderr, "Failed add the netfn and others : %s %s\n", __FUNCTION__, strerror(-r));<br>> +        goto final;<br>>      }<br>><br>>      r = sd_bus_message_append_array(m, 'y', buf, len);<br>>      if (r < 0) {<br>> -        fprintf(stderr, "Failed to add the string of response bytes: %s\n", strerror(-r));<br>> -        return -1;<br>> +        fprintf(stderr, "Failed to add the string of response bytes: %s %s\n", __FUNCTION__, strerror(-r));<br>> +        goto final;<br>>      }<br>><br>><br>> @@ -213,18 +213,19 @@ static int send_ipmi_message(sd_bus_message *req, unsigned char seq, unsigned ch<br>>      // Call the IPMI responder on the bus so the message can be sent to the CEC<br>>      r = sd_bus_call(bus, m, 0, &error, &reply);<br>>      if (r < 0) {<br>> -        fprintf(stderr, "Failed to call the method: %s", strerror(-r));<br>> -        return -1;<br>> +        fprintf(stderr, "Failed to call the method: %s %s\n", __FUNCTION__, strerror(-r));<br>> +        goto final;<br>>      }<br>><br>>      r = sd_bus_message_read(reply, "x", &pty);<br>>      if (r < 0) {<br>> -       fprintf(stderr, "Failed to get a rc from the method: %s\n", strerror(-r));<br>> -<br>> +       fprintf(stderr, "Failed to get a rc from the method: %s %s\n", __FUNCTION__, strerror(-r));<br>>      }<br>><br>> +final:<br>>      sd_bus_error_free(&error);<br>>      sd_bus_message_unref(m);<br>> +    sd_bus_message_unref(reply);<br>><br>><br>>      return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS;<br>> diff --git a/sensorhandler.C b/sensorhandler.C<br>> index dd83152..131da71 100644<br>> --- a/sensorhandler.C<br>> +++ b/sensorhandler.C<br>> @@ -29,6 +29,7 @@ sensorTypemap_t g_SensorTypeMap[] = {<br>>      {0xe9, 0x09, "OccStatus"},  // E9 is an internal mapping to handle sensor type code os 0x09<br>>      {0xC3, 0x6F, "BootCount"},<br>>      {0x1F, 0x6F, "OperatingSystemStatus"},<br>> +    {0x12, 0x6F, "SYSTEM"},<br>>      {0xFF, 0x00, ""},<br>>  };<br>><br>> @@ -39,6 +40,14 @@ struct sensor_data_t {<br>>  }  __attribute__ ((packed)) ;<br>><br>><br>> +struct sensor_reading_ret_t {<br>> +    uint8_t reading;<br>> +    uint8_t options;<br>> +    uint8_t assert[2];<br>> +}  __attribute__ ((packed)) ;<br>> +<br>> +<br>> +<br><br>Whitespace<br><br>>  uint8_t dbus_to_sensor_type(char *p) {<br>><br>>      sensorTypemap_t *s = g_SensorTypeMap;<br>> @@ -155,6 +164,42 @@ ipmi_ret_t ipmi_sen_set_sensor(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>>      return rc;<br>>  }<br>><br>> +<br><br>Whitespace<br><br>> +ipmi_ret_t ipmi_sen_get_sensor_reading(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> +                             ipmi_request_t request, ipmi_response_t response,<br>> +                             ipmi_data_len_t data_len, ipmi_context_t context)<br>> +{<br>> +    sensor_data_t *reqptr = (sensor_data_t*)request;<br>> +    sensor_reading_ret_t ret;<br>> +    ipmi_ret_t rc;<br>> +<br>> +    printf("IPMI GET_SENSOR_READING [0x%02x]\n",reqptr->sennum);<br>> +<br>> +<br><br>Whitespace<br><br>> +    // TODO Call dbus method to get teh real state of the sensor and<br>> +    // translate it to IPMI.  For now stop every boot of a Palmetto<br>> +    // from creating error logs about missing the Reboot Sensor.<br>> +    if (reqptr->sennum == 9) {<br><br>Give the magic number a name.<br><br>> +        ret.reading = 2;<br><br>Again, what does 2 mean?<br><br>> +        ret.options = 0;<br>> +        ret.assert[0] = 0;<br>> +        ret.assert[1] = 0;<br>> +<br>> +        *data_len = sizeof(ret);<br>> +        memcpy(response, &ret, *data_len);<br><br>Is there a reason why you create the local ret, and then copy the<br>results in to response? Could you just put the data straight into<br>response?<br><br>> +        rc = IPMI_CC_OK;<br>> +<br>> +    } else {<br>> +        *data_len = 0;<br>> +        rc = IPMI_CC_SENSOR_INVALID;<br>> +    }<br>> +<br>> +<br><br>Whitespace<br><br>> +    return rc;<br>> +}<br>> +<br>> +<br>> +<br><br>Whitespace.<br><br>>  ipmi_ret_t ipmi_sen_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>>                               ipmi_request_t request, ipmi_response_t response,<br>>                               ipmi_data_len_t data_len, ipmi_context_t context)<br>> @@ -179,5 +224,8 @@ void register_netfn_sen_functions()<br>>      printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_SENSOR, IPMI_CMD_SET_SENSOR);<br>>      ipmi_register_callback(NETFUN_SENSOR, IPMI_CMD_SET_SENSOR, NULL, ipmi_sen_set_sensor);<br>><br>> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_SENSOR, IPMI_CMD_GET_SENSOR_READING);<br>> +    ipmi_register_callback(NETFUN_SENSOR, IPMI_CMD_GET_SENSOR_READING, NULL, ipmi_sen_get_sensor_reading);<br>> +<br>>      return;<br><br>This is redundant.<br><br>>  }<br>> diff --git a/sensorhandler.h b/sensorhandler.h<br>> index 7b89a18..dd940dc 100644<br>> --- a/sensorhandler.h<br>> +++ b/sensorhandler.h<br>> @@ -6,6 +6,7 @@<br>>  // IPMI commands for net functions.<br>>  enum ipmi_netfn_sen_cmds<br>>  {<br>> +    IPMI_CMD_GET_SENSOR_READING = 0x2D,<br>>      IPMI_CMD_GET_SENSOR_TYPE = 0x2F,<br>>      IPMI_CMD_SET_SENSOR      = 0x30,<br>>  };<br>> diff --git a/storageaddsel.C b/storageaddsel.C<br>> index 345dfd5..991c78d 100644<br>> --- a/storageaddsel.C<br>> +++ b/storageaddsel.C<br>> @@ -155,7 +155,7 @@ int send_esel_to_dbus(const char *desc, const char *sev, const char *details, ui<br>>         sd_bus *mbus = NULL;<br>>      sd_bus_error error = SD_BUS_ERROR_NULL;<br>>      sd_bus_message *reply = NULL, *m=NULL;<br>> -    uint16_t *pty;<br>> +    uint16_t x;<br><br>Use a more descriptive name than "x".<br><br>>      int r;<br>><br>>      mbus = ipmid_get_sd_bus_connection();<br>> @@ -181,26 +181,22 @@ int send_esel_to_dbus(const char *desc, const char *sev, const char *details, ui<br>>          fprintf(stderr, "Failed to add the raw array of bytes: %s\n", strerror(-r));<br>>          return -1;<br>>      }<br>> -<br>>      // Call the IPMI responder on the bus so the message can be sent to the CEC<br>>      r = sd_bus_call(mbus, m, 0, &error, &reply);<br>>      if (r < 0) {<br>> -        fprintf(stderr, "Failed to call the method: %s", strerror(-r));<br>> +        fprintf(stderr, "Failed to call the method: %s %s\n", __FUNCTION__, strerror(-r));<br><br>Systemd gives us a string in error->message. You could print that<br>instead of strerror.<br><br>>          return -1;<br>>      }<br>> -<br>> -    r = sd_bus_message_read(reply, "q", &pty);<br>> +    r = sd_bus_message_read(reply, "q", &x);<br>>      if (r < 0) {<br>>          fprintf(stderr, "Failed to get a rc from the method: %s\n", strerror(-r));<br>> -    } else {<br>> -        r = *pty;<br>>      }<br>><br>> +finish:<br><br>storageaddsel.C:195:1: warning: label ‘finish’ defined but not used<br>[-Wunused-label]<br> finish:<br> ^<br><br>>      sd_bus_error_free(&error);<br>>      sd_bus_message_unref(m);<br>>      sd_bus_message_unref(reply);<br>> -<br>> -    return r;<br>> +    return 0;<br><br>This function will print an error message when sd_bus_message_read<br>fails, but will always return zero now. Is that the intention?<br><br>>  }<br>><br>><br>> @@ -213,41 +209,27 @@ void send_esel(uint16_t recordid) {<br>>         int r;<br>><br>>         uint8_t hack[] = {0x30, 0x32, 0x34};<br>> -<br>>         asprintf(&path,"%s%04x", "/tmp/esel", recordid);<br>> -<br>>         sz = getfilestream(path, &buffer);<br>> -<br>>         if (sz == 0) {<br>>                 printf("Error file does not exist %d\n",__LINE__);<br>>                 free(path);<br>>                 return;<br>>         }<br>><br>> -<br>>         sev = create_esel_severity(buffer);<br>> -<br>>         create_esel_association(buffer, &assoc);<br>> -<br>>         create_esel_description(buffer, sev, &desc);<br>><br>> -<br>>         // TODO until ISSUE <a href="https://github.com/openbmc/rest-dbus/issues/2">https://github.com/openbmc/rest-dbus/issues/2</a><br>>         // I cant send extended ascii chars.  So 0,2,4 for now...<br>> -       r = send_esel_to_dbus(desc, sev, assoc, hack, 3);<br>> -<br>> -       asprintf(&pathsent,"%s_%d", path, r);<br>> -<br>> -<br>> -       rename(path, pathsent);<br><br>storageaddsel.C: In function ‘void send_esel(uint16_t)’:<br>storageaddsel.C:209:6: warning: unused variable ‘r’ [-Wunused-variable]<br>  int r;<br>      ^<br>storageaddsel.C:229:16: warning: ‘pathsent’ may be used uninitialized<br>in this function [-Wmaybe-uninitialized]<br>  free(pathsent);<br>                ^<br>storageaddsel.C:207:15: note: ‘pathsent’ was declared here<br>  char *path, *pathsent;<br>               ^<br><br>> +       send_esel_to_dbus(desc, sev, assoc, hack, 3);<br>><br>>         free(path);<br>>         free(pathsent);<br>>         free(assoc);<br>>         free(desc);<br>> -<br>>         delete[] buffer;<br>><br>> -<br>>         return;<br>>  }<br>> --<br>> 2.6.3<br>><br>><br>> _______________________________________________<br>> openbmc mailing list<br>> <a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br>> <a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a><br>_______________________________________________<br>openbmc mailing list<br><a target="_blank" href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a><br></font></div></openbmc-bounces+austenc=us.ibm.com@lists.ozlabs.org></joel@jms.id.au></div></div></font><BR>