[PATCH openpower-host-ipmi-oem v2 3/3] Fixes from Joel review
Joel Stanley
joel at jms.id.au
Fri Oct 16 15:20:13 AEDT 2015
On Fri, Oct 16, 2015 at 6:28 AM, OpenBMC Patches <patches at stwcx.xyz> wrote:
> From: Chris Austen <austenc at us.ibm.com>
>
> ---
> Makefile | 8 +++----
> ipmid-api.h | 4 ++--
> oemhandler.C | 34 ++++++++++++++---------------
> testit.C | 70 ++++++++++++++++++++++++------------------------------------
> 4 files changed, 51 insertions(+), 65 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index e2c405b..e2ed670 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -6,14 +6,14 @@ TESTER_OBJ = testit.o oemhandler.o
> LIB_OEM_OBJ = oemhandler.o
> LIB_OEM = liboemhandler.so
>
> -INC_FLAG += -I. -O2 --std=gnu++11
> -LIB_FLAG += -rdynamic
> +LDFLAGS += -rdynamic
> +CXXFLAGS += -fpic -Wall
>
>
> all: $(TESTER) $(LIB_OEM)
>
> %.o: %.C
> - $(CXX) -fpic -c $< $(CXXFLAGS) $(INC_FLAG) $(IPMID_PATH) -o $@
> + $(CXX) -fpic -c $< $(CXXFLAGS) -o $@
fpic is here twice.
Once you remove this, the rule is same as the built in rule, so you
don't need it.
>
>
> $(LIB_OEM): $(LIB_OEM_OBJ)
> @@ -24,4 +24,4 @@ $(TESTER): $(TESTER_OBJ)
> $(CXX) $^ $(LDFLAGS) $(LIB_FLAG) -o $@ -ldl
LIB_FLAG does not exist anymore.
-ldl can go in the LDFLAGS.
>
> clean:
> - rm -f $(TESTER) *.o *.so
> + $(RM) $(LIB_OEM_OBJ) $(LIB_OEM) $(TESTER_OBJ) $(TESTER)
> diff --git a/ipmid-api.h b/ipmid-api.h
> index ef2be38..68090f3 100644
> --- a/ipmid-api.h
> +++ b/ipmid-api.h
> @@ -1,5 +1,5 @@
> -#ifndef __HOST_IPMID_IPMI_COMMON_H__
> -#define __HOST_IPMID_IPMI_COMMON_H__
> +#ifndef __HOST_IPMID_API_COMMON_H__
> +#define __HOST_IPMID_API_COMMON_H__
> #include <stdlib.h>
>
> // length of Completion Code and its ALWAYS _1_
> diff --git a/oemhandler.C b/oemhandler.C
> index c4eae21..fa1b9a2 100644
> --- a/oemhandler.C
> +++ b/oemhandler.C
> @@ -5,8 +5,6 @@
>
> void register_netfn_oem_partial_esel() __attribute__((constructor));
>
> -
> -
> const char *g_esel_path = "/tmp/";
> uint16_t g_record_id = 0x0100;
>
> @@ -32,39 +30,41 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> {
> esel_request_t *reqptr = (esel_request_t*) request;
> FILE *fp;
> - char string[32];
> - short offset = 0, record=0;
> - unsigned short rlen;
> + short offset = 0;
> + uint8_t rlen;
> ipmi_ret_t rc = IPMI_CC_OK;
> - char iocmd[4];
> + char iocmd[][4] = { { "wb" }, {"rb+"} };
> + char string[64];
> + char *pio;
>
> - strcpy(string, g_esel_path);
>
> offset = LSMSSWAP(reqptr->offsetls, reqptr->offsetms);
>
> - sprintf(string, "%s%s%02x%02x", string, "esel", reqptr->selrecordms, reqptr->selrecordls);
> + snprintf(string, sizeof(string), "%s%s%02x%02x", g_esel_path, "esel", reqptr->selrecordms, reqptr->selrecordls);
>
>
> // Length is the number of request bytes minus the header itself.
> // The header contains an extra byte to indicate the start of
> // the data (so didn't need to worry about word/byte boundaries)
> // hence the -1...
> - rlen = ((unsigned short)*data_len) - (sizeof(esel_request_t));
> + rlen = (*data_len) - (uint8_t) (sizeof(esel_request_t));
>
>
> printf("IPMI PARTIAL ESEL for %s Offset = %d Length = %d\n",
> string, offset, rlen);
>
> +
> + // Rules state for a Partial eSel that the first write of a
> + // new esel will be the sensor data record. We will use that
> + // to indicate this is a new record rather then an ofset in
> + // my next commit TODO
> if (offset == 0) {
> - strcpy(iocmd, "wb");
> + pio = iocmd[0];
pio = "wb"
> } else {
> - // I was thinking "ab+" but it appears it doesn't
> - // do what fseek asks. Modify to rb+ and fseek
> - // works great...
> - strcpy(iocmd, "rb+");
> + pio = iocmd[1];
pio = "rb+"
Then you can lose the char array.
> }
>
> - if ((fp = fopen(string, iocmd)) != NULL) {
> + if ((fp = fopen(string, pio)) != NULL) {
> fseek(fp, offset, SEEK_SET);
> fwrite(reqptr+1,rlen,1,fp);
> fclose(fp);
> @@ -75,8 +75,8 @@ ipmi_ret_t ipmi_ibm_oem_partial_esel(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>
>
> } else {
> - fprintf(stderr, "Error trying to perform %s for esel%s\n",iocmd, string);
> - ipmi_ret_t rc = IPMI_CC_INVALID;
> + fprintf(stderr, "Error trying to perform %s for esel%s\n",pio, string);
> + rc = IPMI_CC_INVALID;
> *data_len = 0;
> }
>
> diff --git a/testit.C b/testit.C
> index b563257..25e069c 100644
> --- a/testit.C
> +++ b/testit.C
> @@ -4,6 +4,12 @@
> #include "oemhandler.h"
> #include <cstring>
>
Whitespace.
> +#include <iostream>
> +#include <fstream>
> +#include <vector>
is there any sorting here? ABC order is generally a good rule.
> +
> +using namespace std;
> +
> const char* g_filepath = "/tmp/";
>
> const char * getFilePath(void) { return g_filepath; }
> @@ -12,56 +18,36 @@ const char * getFilePath(void) { return g_filepath; }
> #define MAXRESPONSE 2
>
>
> -// Returns the length of the file
> -size_t checkFileSize(const char *fn) {
>
> - FILE *fp;
> - size_t len = -1;
> -
> - if ((fp = fopen(fn, "rb")) != NULL) {
> - fseek(fp, 0, SEEK_END);
> - len = ftell(fp);
> - fclose(fp);
> - }
> -
> - return len;
> +// Returns the length of the file
> +std::ifstream::pos_type filesize(const char* filename)
> +{
> + std::ifstream in(filename, std::ifstream::ate | std::ifstream::binary);
> + return in.tellg();
> }
>
> +
> // Compares a string to the data in a file.
> // Returns 0 if complete match
> -int compareData(const char *fn, const char *string, size_t len) {
> +int compareData(const char *filename, const char *string, size_t len)
> +{
>
> - int rc = 0;
> - FILE *fp;
> - char fbyte;
> - int i=0;
> + std::ifstream in(filename, std::ifstream::ate | std::ifstream::binary);
> + std::streamsize size = in.tellg();
>
> - if ((fp = fopen(fn, "rb")) != NULL) {
> + std::vector<char> buffer(size);
> +
> + in.read(buffer.data(), size);
> +
> + if (!std::memcmp(string, buffer.data(), len))
> + return -1;
>
> - while(fread(&fbyte, 1,1,fp) == 1) {
> - if (*(string+i) != fbyte) {
> - rc = -2;
> - break;
> - }
> - i++;
> - }
> - fclose(fp);
> -
> - if (len != i) {
> - rc = -3;
> - }
> -
> - } else {
> - rc = -1;
> - }
> -
> -
> -
> - return rc;
> +
> + return 0;
> }
>
>
More information about the openbmc
mailing list