[PATCH phosphor-host-ipmid] Allow esels to be sent to dbus event log

[Patrick Williams]

On Sun, Oct 25, 2015 at 08:40:05PM -0500, OpenBMC Patches wrote:
> From: Chris Austen <austenc at us.ibm.com>
> diff --git a/storageaddsel.C b/storageaddsel.C
> new file mode 100644
> index 0000000..f11cf00
> --- /dev/null
> +++ b/storageaddsel.C
> @@ -0,0 +1,280 @@
> +#include <stdint.h>
> +#include <cstdlib>
> +#include <cstring>
> +#include <fstream>
> +#include <iostream>
> +#include <vector>
> +#include <systemd/sd-bus.h>
> +
> +#include "ipmid.H"
> +#include "storagehandler.h"
> +#include "sensorhandler.h"
> +
> +using namespace std;
> +
> +extern int find_openbmc_path(const char *, const uint8_t , dbus_interface_t *);
> +
> +
> +//////////////////////////
> +struct esel_section_headers_t {
> +	uint8_t sectionid[2];
> +	uint8_t sectionlength[2];
> +	uint8_t version;
> +	uint8_t subsectiontype;
> +	uint8_t compid;

There is a mixture of tabs and not-tabs in this file.  Can you set your
editor to expand tabs?

> +};
> +
> +#define MAXSEVDESC 64
> +struct severity_values_t {
> +	uint8_t type;

Is there any need to specify the size of this?

> +	char    description[MAXSEVDESC];

Should be const char* instead of char[].  No advantage to specifying an array
length here, except for possibility of accidentally having
non-terminated strings.

> +};
> +
> +
> +severity_values_t g_sev_desc[] = {
> +	{0x10, "recoverable error"},
> +	{0x20, "predictive error"},
> +	{0x40, "unrecoverable error"},
> +	{0x50, "critical error"},
> +	{0x60, "error from a diagnostic test"},
> +	{0x70, "recovered symptom "},
> +	{0xFF, "Unknown"},
> +};
> +
> +//0000 df00 0000 0020 0004 0035 6faa 0000
> +
> +
> +char *sev_lookup(uint8_t  n) {

Function should return const char*.

> +
> +	severity_values_t *p = g_sev_desc;
> +
> +	while (p->type != 0xFF)  {
> +		if (p->type == n) {
> +			break;
> +		}
> +		p++;
> +	}
> +				
> +	return p->description;
> +}

The C++11/14 way of doing this is:

    struct severity_values_t
    {
        uint8_t type;
        const char* description;
    };

    const std::vector<severity_values_t> g_sev_desc = {
        {0x10, "recoverable error"},
        {0x20, "predictive error"},
        {0x40, "unrecoverable error"},
        {0x50, "critical error"},
        {0x60, "error from a diagnostic test"},
    };

    const char* sev_lookup(uint8_t n)
    {
        auto i = std::find_if(std::begin(g_sev_desc), std::end(g_sev_desc),
                [n](auto& e){ return e.type == n; });
        return i->description;
    }

You don't need special terminators this way and you know the find_if is bug
proof.  You can also fairly easily change to binary_search if needed.

The '[n](auto& e){ return e.type == n; }' is a "lambda function".
[n](auto e) says that we are creating a lambda function that captures
the local variable 'n' and takes in an auto-typed reference to the
element under consideration.

> +
> +
> +
> +int find_sensor_type_string(uint8_t sensor_number, char **s) {
> +
> +    dbus_interface_t a;
> +    char *p;
> +    char r;
> +
> +    r = find_openbmc_path("SENSOR", sensor_number, &a);
> +
> +    if ((r < 0) || (a.bus[0] == 0)) {
> +
> +    	// Just make a generic message for errors that 
> +    	// occur on sensors that dont exist
> +    	asprintf(s, "Unknown Sensor (0x%02x)", sensor_number);
> +
> +    } else {
> +
> +		p = strrchr (a.path, '/');

Need to check for nullptr here.

> +		asprintf(s, "%s", p+1);
> +    }
> +
> +    return 0;
> + }
> +

> +char *create_esel_severity(const uint8_t *buffer) {

const char*

> +int create_esel_buffer_hack(const uint8_t *buffer, size_t sz, char **ascii) {
> +
> +	uint8_t *p	= new uint8_t [sz*2];

It is better to use unique_ptr instead of new.  Since this is a variable
length array you can do 'unique_ptr<uint8_t[]>(new uint8_t [sz*2])' but
if it was a non-array or non-variable-array you could do
'unique_ptr<foo>()'.  The advantage of 'unique_ptr' is you don't need to
worry about delete because it automatically deletes when going out of
scope.

> +	int i, j;

Should favor size_t for iterator types.

-- 

Most of this is clean-up.  I'm fine if we merge this now and have a
follow-up commit for the clean-ups.  Let me know.

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151027/c7562ce0/attachment-0001.sig>