[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;
> +};
> +
> +#define MAXSEVDESC 64
> +struct severity_values_t {
> +	uint8_t type;
> +	char    description[MAXSEVDESC];
> +};

There really isn't much advantage to using uint8_t here, is there?

Description should just be a const char*.  No advantage to trying to
limit the array like that, other than having an unintended missing null
character.

> +
> +
> +severity_values_t g_sev_desc[] = {

Array should be const.

> +	{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/C++14 way to do this is as follows:

    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 p){ return p.type == n; });
        return i->description;
    }

The [n](auto p){ return p.type == n;} is called a lambda function.

[n] says "bind the local variable n into the lambda function"
(auto p) means the function will have 1 parameter: the iterator.


> +
> +
> +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);

You're starting to get a mixture of tabs and spaces.  What is your tab size?

> +		p = strrchr (a.path, '/');
> +		asprintf(s, "%s", p+1);

Need to check for NULL from strrchr?

> +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];

unique_ptr<uint8_t[]>(new uint8_t[sz*2]) is better C++ now because you don't
need to delete at the end.

> +	int i, j;

size_t is better for iterators.

--

Most of this code review is changing to 'const' and other minor cleanups. I'm
ok if you want to merge this as is and have a follow-up commit for the
cleanups.

-- 
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/6b20d320/attachment.sig>