[PATCH phosphor-host-ipmid] Moved from gdbus to sdbus. Fixed sdbus memory leak. Added commands to get a Palmetto to IPL further
Patrick Williams
patrick at stwcx.xyz
Tue Oct 13 06:35:26 AEDT 2015
Best practice would be to break this down into a few smaller commits.
At least one for 'move to sdbus' and one for 'add storage handler'.
On Sun, Oct 11, 2015 at 07:33:49PM -0500, OpenBMC Patches wrote:
> From: Chris Austen <austenc at us.ibm.com>
>
> ---
>
> diff --git a/Makefile b/Makefile
> index 4baf209..929ef21 100755
> --- a/Makefile
> +++ b/Makefile
> @@ -2,19 +2,36 @@ CXX ?= $(CROSS_COMPILE)g++
>
> DAEMON = ipmid
> DAEMON_OBJ = $(DAEMON).o
> -LIB_OBJ = apphandler.o
> -LIBS = libapphandler.so
> +LIB_APP_OBJ = apphandler.o
> +LIB_SEN_OBJ = sensorhandler.o
> +LIB_STORAGE_OBJ = storagehandler.o
> +LIB_OEM_OBJ = oemhandler.o
>
> -INC_FLAG += $(shell pkg-config --cflags glib-2.0 gio-unix-2.0) -I. -O2 --std=gnu++11
> -LIB_FLAG += $(shell pkg-config --libs glib-2.0 gio-unix-2.0) -rdynamic
> +LIB_APP = libsenhandler.so
> +LIB_SEN = libapphandler.so
> +LIB_STORAGE = libstoragehandler.so
> +LIB_OEM = liboemhandler.so
The intention was to break these out into separate packages, especially
the OEM one. I know we haven't set up a *-devel pacakge yet though.
> --- a/apphandler.C
> +++ b/apphandler.C
> +ipmi_ret_t ipmi_app_get_bt_capabilities(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)
...
> - unsigned char str[] = {0x00, 0x01, 0xFE, 0xFF, 0x0A, 0x01};
> + unsigned char str[] = {0x01, MAX_IPMI_BUFFER, MAX_IPMI_BUFFER, 0x0A, 0x01};
Why did the original have 0xFE and 0xFF instead of both the same?
> +
> +typedef struct {
> + unsigned char t_use;
> + unsigned char t_action;
> + unsigned char preset;
> + unsigned char flags;
> + unsigned char ls;
> + unsigned char ms;
> +} SET_WD_DATA ;
> +
Use C++/C99-style struct definition. No need for typedef.
> +ipmi_ret_t ipmi_app_set_watchdog(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)
> +{
> +
> + SET_WD_DATA *reqptr = (SET_WD_DATA*) request;
Is this endian-safe?
> diff --git a/ipmid-api.h b/ipmid-api.h
> index 615e0d5..03e1cb6 100755
> --- a/ipmid-api.h
> +++ b/ipmid-api.h
> @@ -70,7 +70,8 @@ enum ipmi_net_fns
> + NETFUN_OEM_IBM = (0x32 << 2)
This shouldn't be in the ipmid-api. It isn't part of the IPMI spec.
Was already removed in the previous review.
> diff --git a/ipmid.C b/ipmid.C
> index 37ac771..38dba91 100644
> --- a/ipmid.C
> +++ b/ipmid.C
> @@ -4,22 +4,82 @@
> +#define FILTER "type='signal',sender='org.openbmc.HostIpmi',member='ReceivedMessage'"
Prefer static const char* or a more unique name.
> +
> + // Add the bytes needed for the methos to obe called
"methos to obe". Is this latin?
> +static int handle_ipmi_command(sd_bus_message *m, void *user_data, sd_bus_error
> + *ret_error) {
...
> + r = sd_bus_message_read_array(m, 'y', &request, &sz );
> + if (r < 0) {
> + fprintf(stderr, "Failed to parse signal message: %s\n", strerror(-r));
> + return -1;
> + }
Is there a read_array_n function to prevent buffer overrun? I can crash
your application by sending a message longer than the size of 'request'.
> + // TODO: Need to pass the buffer length of the request bytes
Isn't this satisfied with 'resplen = sz' below? I'm fine with another
parameter for the input buffer size. I think the intent was that
'resplen' is both input and output size.
> + // Probably should just pass an ipmi object with all the
> + // interesting stuff in there and just one parameter.
Generally, passing as parameters is more efficient. Constructing an
extra object on the stack is just overhead.
> + // Also this is not thread safe at all. response is a single buffer
> + // used by all commands. Seems wrong
Do we have any threads? The current implementation handles only one
command at a time and I don't think we intended to change that.
> diff --git a/ipmid.H b/ipmid.H
> index 6b7d2c9..ab8b030 100755
> --- a/ipmid.H
> +++ b/ipmid.H
> -#define MAX_IPMI_BUFFER 255
> +#define MAX_IPMI_BUFFER 64
Does Hostboot not handle if this is longer than 64?
> diff --git a/oemhandler.C b/oemhandler.C
The OEM messages should be in a separate package.
> +typedef struct {
> + unsigned char residls;
> + unsigned char residms;
> + unsigned short selrecord;
> + unsigned short offset;
> + unsigned char progress;
> + unsigned char data;
> +
> +} ESEL_DATA ;
C++/C99-style struct def.
> +
> +
> +// This function creates a /tmp/esel# file to store the
> +// incoming partial esel. It is the role of some other
> +// function to commit the error log in to long term
> +// storage. Likely via the ipmi add_sel command.
> +ipmi_ret_t ipmi_ibm_oem_partial_esel(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)
> +{
> + ESEL_DATA *reqptr = (ESEL_DATA*) request;
Have concerns with endianness safety on this one especially because the
entries are of mismatched sizes. I think you at least need a
__attribute__(packed) on the struct.
> + if (reqptr->offset == 0) {
> + strcpy(iocmd, "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+");
> + }
Writing to offset 0 is always going to cause truncation (erase of
previous data) while others allow append. Is this intentional? Is
there ever a case where an endpoint is going to write offset zero to
update data after they've written other data?
> +
> + if ((fp = fopen(string, iocmd)) != NULL) {
> + fseek(fp, reqptr->offset, SEEK_SET);
> + fwrite(&reqptr->data,rlen,1,fp);
> + fclose(fp);
No handling return codes from these functions.
Does 'fseek' work if the file isn't already reqptr->offset bytes long?
As in, can I fseek past the current end of the file, and do those bytes
end up automatically filled in with zeros?
> diff --git a/sensorhandler.C b/sensorhandler.C
> new file mode 100644
> index 0000000..3bef067
...
> +typedef struct {
> + unsigned char sennum;
> +} SENSOR_DATA ;
C99/C++ style struct.
> +unsigned char g_sensortype [][2] = {
Best to sort these numerically or use a C++11-style static initialized
map. Then you can use a binary search on the data.
http://stackoverflow.com/questions/2636303/how-to-initialize-a-private-static-const-map-in-c/8688615#8688615
> +unsigned char findSensor(char sensor_number) {
> +
> + int i=0;
> +
> + // TODO : This function should actually call
> + // a dbus object and have it return the data
> + // it is not ready yet so use a Palmetto
> + // based lookup table for now. The g_sensortype
> + // can be removed once the dbus method exists
> + while (g_sensortype[i][0] != 0xff) {
> + if (g_sensortype[i][1] == sensor_number) {
> + break;
> + } else {
> + i++;
> + }
> +
> + }
> +
> + return g_sensortype[i][0];
> +
> +}
Use find, lower_bound, or map lookup. Should avoid writing your own
loops for searching.
> diff --git a/storagehandler.C b/storagehandler.C
> new file mode 100644
> index 0000000..085b442
> --- /dev/null
> +++ b/storagehandler.C
> +typedef struct {
> + unsigned char frunum;
> + unsigned char offsetls;
> + unsigned char offsetms;
> + unsigned char data;
> +} WRITE_FRU_DATA ;
Same comment here as earlier pattern.
> +
> +ipmi_ret_t ipmi_storage_write_fru_data(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)
> +{
> + WRITE_FRU_DATA *reqptr = (WRITE_FRU_DATA*) request;
Same comment here as earlier pattern.
> + // I was thinking "ab+" but it appears it doesn't
> + // do what fseek asks. Modify to rb+ and fseek
> + // works great...
> + if (offset == 0) {
> + strcpy(iocmd, "wb");
> + } else {
> + strcpy(iocmd, "rb+");
> + }
> +
> + if ((fp = fopen(string, iocmd)) != NULL) {
> + fseek(fp, offset, SEEK_SET);
> + fwrite(&reqptr->data,rlen,1,fp);
> + fclose(fp);
Same comments here on fopen and return codes.
--
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/20151012/72f9a718/attachment.sig>
More information about the openbmc
mailing list