[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