[PATCH phosphor-host-ipmid] Alignment and double error
Chris Austen
austenc at us.ibm.com
Thu Dec 3 13:43:21 AEDT 2015
I removed the sensor reading feature. I'll use a separate commit for that in the next sprint.
Chris Austen
POWER Systems Enablement Manager
(512) 286-5184 (T/L: 363-5184)
-----"openbmc" <openbmc-bounces+austenc=us.ibm.com at lists.ozlabs.org> wrote: -----
To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
From: Joel Stanley
Sent by: "openbmc"
Date: 12/02/2015 06:57PM
Cc: OpenBMC Maillist <openbmc at lists.ozlabs.org>
Subject: Re: [PATCH phosphor-host-ipmid] Alignment and double error
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
_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151203/248eef38/attachment-0001.html>
More information about the openbmc
mailing list