[PATCH phosphor-host-ipmid v3] Add get/set ipmid command support with correct DBUS property handling.
Cyril Bur
cyrilbur at gmail.com
Mon Jan 4 17:38:54 AEDT 2016
On Tue, 22 Dec 2015 20:30:27 -0600
OpenBMC Patches <openbmc-patches at stwcx.xyz> wrote:
> From: shgoupf <shgoupf at cn.ibm.com>
>
Hi shgoupf,
The patch needs work, unfortunately it appears there are some problems you've
inherited here but lets not continue them.
I've outlined some concerns below.
> 1) Two methods to handle the dbus property set and get:
> a) dbus_set_property()
> b) dbus_get_property()
> 2) The property is stored as a 10 character strings which representsd 5-byte information.
> 3) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.
> 4) Get service name via object mapper
> a) The connection name is got via objectmapper.
> b) The method used to get the connection name is object_mapper_get_connection().
> c) dbus_get_property/dbus_set_property will get the connection name via the above method instead of hard coding.
> 5) Error code are properly handled.
> 6) Use sprinf/sscanf for int/string conversion.
> a) Instead of reinventing the wheel by defining methods converting int to string, use sprintf/sscanf should be a more clean and robust way.
> ---
> chassishandler.C | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> chassishandler.h | 17 +++
> 2 files changed, 325 insertions(+), 12 deletions(-)
>
> diff --git a/chassishandler.C b/chassishandler.C
> index 1389db9..d68b5a8 100644
> --- a/chassishandler.C
> +++ b/chassishandler.C
> @@ -9,6 +9,205 @@ const char *chassis_bus_name = "org.openbmc.control.Chassis";
> const char *chassis_object_name = "/org/openbmc/control/chassis0";
> const char *chassis_intf_name = "org.openbmc.control.Chassis";
>
> +// Host settings in dbus
> +// Service name should be referenced by connection name got via object mapper
> +// const char *settings_service_name = "org.openbmc.settings.Host";
> +const char *settings_object_name = "/org/openbmc/settings/host0";
> +const char *settings_intf_name = "org.freedesktop.DBus.Properties";
> +
> +const char *objmapper_service_name = "org.openbmc.objectmapper";
> +const char *objmapper_object_name = "/org/openbmc/objectmapper/objectmapper";
> +const char *objmapper_intf_name = "org.openbmc.objectmapper.ObjectMapper";
> +
> +int object_mapper_get_connection(char** buf, const char* obj_path)
> +{
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + sd_bus_message *m = NULL;
> + sd_bus *bus = NULL;
> + char *temp_buf = NULL, *intf = NULL;
> + size_t buf_size = 0;
> + int r;
> +
> + // Open the system bus where most system services are provided.
> + r = sd_bus_open_system(&bus);
> + if (r < 0) {
> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + // Bus, service, object path, interface and method are provided to call
> + // the method.
> + // Signatures and input arguments are provided by the arguments at the
> + // end.
Multiline comments should use the multiline comment style /* */, so:
/*
* Bus, service, object path, interface and method are provided to call
* the method.
* Signatures and input arguments are provided by the arguments at the
* end.
*/
> + r = sd_bus_call_method(bus,
> + objmapper_service_name, /* service to contact */
> + objmapper_object_name, /* object path */
> + objmapper_intf_name, /* interface name */
> + "GetObject", /* method name */
> + &error, /* object to return error in */
> + &m, /* return message on success */
> + "s", /* input signature */
> + obj_path /* first argument */
> + );
> +
> + if (r < 0) {
> + fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> + goto finish;
> + }
> +
> + // Get the key, aka, the connection name
> + sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> + // TODO: check the return code. Currently for no reason the message
> + // parsing of object mapper is always complaining about
> + // "Device or resource busy", but the result seems OK for now. Need
> + // further checks.
So how sure are you that what you're doing now is robust?
> + //r = sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);
> + //if (r < 0) {
> + // fprintf(stderr, "Failed to parse response message: %s\n", strerror(-r));
> + // goto finish;
> + //}
Is there a reason you're sending commented code?
> +
> + buf_size = strlen(temp_buf) + 1;
> + printf("IPMID connection name: %s\n", temp_buf);
> + *buf = (char*)malloc(buf_size);
> +
> + if (*buf == NULL) {
> + fprintf(stderr, "Malloc failed for get_sys_boot_options");
> + r = -1;
> + goto finish;
> + }
> +
> + memcpy(*buf, temp_buf, buf_size);
> +
> +finish:
> + sd_bus_error_free(&error);
> + sd_bus_message_unref(m);
> + sd_bus_unref(bus);
> +
> + return r;
> +}
> +
> +int dbus_get_property(char* buf)
> +{
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + sd_bus_message *m = NULL;
> + sd_bus *bus = NULL;
> + char* temp_buf = NULL;
> + uint8_t* get_value = NULL;
> + char* connection = NULL;
> + int r, i;
> +
> + r = object_mapper_get_connection(&connection, settings_object_name);
> + if (r < 0) {
> + fprintf(stderr, "Failed to get connection, return value: %d.\n", r);
> + goto finish;
> + }
> +
> + printf("connection: %s\n", connection);
> +
> + // Open the system bus where most system services are provided.
> + r = sd_bus_open_system(&bus);
> + if (r < 0) {
> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + // Bus, service, object path, interface and method are provided to call
> + // the method.
> + // Signatures and input arguments are provided by the arguments at the
> + // end.
> + r = sd_bus_call_method(bus,
> + connection, /* service to contact */
> + settings_object_name, /* object path */
> + settings_intf_name, /* interface name */
> + "Get", /* method name */
> + &error, /* object to return error in */
> + &m, /* return message on success */
> + "ss", /* input signature */
> + settings_intf_name, /* first argument */
> + "boot_flags"); /* second argument */
> +
> + if (r < 0) {
> + fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> + goto finish;
> + }
> +
> + // The output should be parsed exactly the same as the output formatting
> + // specified.
> + r = sd_bus_message_read(m, "v", "s", &temp_buf);
> + if (r < 0) {
> + fprintf(stderr, "Failed to parse response message: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + printf("IPMID boot option property get: {%s}.\n", (char*) temp_buf);
> +
> + memcpy(buf, temp_buf, 2*NUM_RETURN_BYTES_OF_GET_USED + 1);
This isn't the only place you do this 2*value + 1, I also don't see a place
you've used NUM_RETURN_BYTES_OF_GET_USED do you just want to make the #define
11? What are you trying to do?
> +
> +finish:
> + sd_bus_error_free(&error);
> + sd_bus_message_unref(m);
> + sd_bus_unref(bus);
> + free(connection);
> +
> + return r;
> +}
> +
> +int dbus_set_property(const char* buf)
> +{
> + sd_bus_error error = SD_BUS_ERROR_NULL;
> + sd_bus_message *m = NULL;
> + sd_bus *bus = NULL;
> + char* connection = NULL;
> + int r;
> +
> + r = object_mapper_get_connection(&connection, settings_object_name);
> + if (r < 0) {
> + fprintf(stderr, "Failed to get connection, return value: %d.\n", r);
> + goto finish;
> + }
> + printf("connection: %s\n", connection);
> +
> + // Open the system bus where most system services are provided.
> + r = sd_bus_open_system(&bus);
> + if (r < 0) {
> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));
> + goto finish;
> + }
> +
> + // Bus, service, object path, interface and method are provided to call
> + // the method.
> + // Signatures and input arguments are provided by the arguments at the
> + // end.
> + r = sd_bus_call_method(bus,
> + connection, /* service to contact */
> + settings_object_name, /* object path */
> + settings_intf_name, /* interface name */
> + "Set", /* method name */
> + &error, /* object to return error in */
> + &m, /* return message on success */
> + "ssv", /* input signature */
> + settings_intf_name, /* first argument */
> + "boot_flags", /* second argument */
> + "s", /* third argument */
> + buf); /* fourth argument */
> +
> + if (r < 0) {
> + fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> + goto finish;
> + }
> +
> + printf("IPMID boot option property set: {%s}.\n", buf);
> +
> +finish:
> + sd_bus_error_free(&error);
> + sd_bus_message_unref(m);
> + sd_bus_unref(bus);
> + free(connection);
> +
> + return r;
> +}
> +
> void register_netfn_chassis_functions() __attribute__((constructor));
>
> struct get_sys_boot_options_t {
> @@ -17,8 +216,25 @@ struct get_sys_boot_options_t {
> uint8_t block;
> } __attribute__ ((packed));
>
> -ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> - ipmi_request_t request, ipmi_response_t response,
> +struct set_sys_boot_options_t {
> + uint8_t parameter;
> + union {
> + struct {
> + uint8_t d1;
> + uint8_t d2;
> + uint8_t d3;
> + uint8_t d4;
> + uint8_t d5;
> + uint8_t d6;
> + uint8_t d7;
> + uint8_t d8;
> + } data8;
> + uint64_t data64;
> + } data;
> +} __attribute__ ((packed));
Ok so I'm opposed to casting an array into a struct however, after having
read part 28.12 - how can you be sure that the data portion is going to be 8
bytes? It looks like you could get zero data...
"Passing 0-bytes of parameter data allows the parameter valid bit to be
changed without affecting the present parameter setting. "
I'm worried.
> +
> +ipmi_ret_t ipmi_chassis_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)
> {
> printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);
> @@ -48,7 +264,7 @@ int ipmi_chassis_power_control(const char *method)
>
> rc = sd_bus_call_method(bus_type, // On the System Bus
> chassis_bus_name, // Service to contact
> - chassis_object_name, // Object path
> + chassis_object_name, // Object path
> chassis_intf_name, // Interface name
> method, // Method to be called
> &bus_error, // object to return error
> @@ -73,8 +289,8 @@ int ipmi_chassis_power_control(const char *method)
> //----------------------------------------------------------------------
> // Chassis Control commands
> //----------------------------------------------------------------------
> -ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> - ipmi_request_t request, ipmi_response_t response,
> +ipmi_ret_t ipmi_chassis_control(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)
> {
> // Error from power off.
> @@ -105,8 +321,8 @@ ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);
> }
>
> -ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
> - ipmi_request_t request, ipmi_response_t response,
> +ipmi_ret_t ipmi_chassis_get_sys_boot_options(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)
> {
> ipmi_ret_t rc = IPMI_CC_OK;
> @@ -116,23 +332,100 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>
> get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;
>
> - // TODO Return default values to OPAL until dbus interface is available
> + char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET*2 + 1);
Why are you mallocing buf, whats wrong with having it on the stack?
See my previous comment about the 2*value + 1, where you've used it twice with
2* +1 and once without, what's going on?
> +
> + if (buf == NULL) {
> + fprintf(stderr, "Malloc failed for get_sys_boot_options.\n");
> + return IPMI_OUT_OF_SPACE;
> + }
> +
> + // Init the buffer to all zero.
> + memset(buf, 0, NUM_RETURN_BYTES_OF_GET*2 + 1);
> +
Might I suggest you use calloc(1, <size>); rather than malloc if you want
zeroed memory?
> + // Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.
> + // This is the only parameter used by petitboot.
> + if (reqptr->parameter == 5)
> + {
> + int r = dbus_get_property(buf);
> +
> + if (r < 0) {
> + fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
If I'm not mistaken you'll have a leak of buf here?
> + return IPMI_OUT_OF_SPACE;
> + }
> +
> + uint64_t return_value;
> + sscanf(buf, "%llX", &return_value);
atoll()?
> +
> + *data_len = NUM_RETURN_BYTES_OF_GET;
> + // TODO: last 3 bytes
> + // (NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless
> + memcpy(response, (uint8_t*)(&return_value), *data_len);
> + free(buf);
> + }
> + else
> + {
> + *data_len = NUM_RETURN_BYTES_OF_GET;
> + // 0x80: parameter not supported
> + buf[0] = 0x80;
#define?
> + memcpy(response, buf, *data_len);
> + free(buf);
> + fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> + return IPMI_CC_PARM_NOT_SUPPORTED;
> + }
> +
> + return rc;
> +}
> +
> +ipmi_ret_t ipmi_chassis_set_sys_boot_options(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)
> +{
> + ipmi_ret_t rc = IPMI_CC_OK;
> +
> + printf("IPMI SET_SYS_BOOT_OPTIONS\n");
> + printf("IPMID set command required return bytes: %i\n", *data_len);
> +
> + set_sys_boot_options_t *reqptr = (set_sys_boot_options_t*) request;
> +
> + char* output_buf = (char*)malloc(NUM_RETURN_BYTES_OF_SET);
> +
> + if (output_buf == NULL) {
> + fprintf(stderr, "Malloc failed for set_sys_boot_options.\n");
> + return IPMI_OUT_OF_SPACE;
> + }
>
You didn't zero the memory here? get/sets should probably do the same in this
respect.
> if (reqptr->parameter == 5) // Parameter #5
> {
> - uint8_t buf[] = {0x1,0x5,80,0,0,0,0};
> - *data_len = sizeof(buf);
> - memcpy(response, &buf, *data_len);
> + char input_buf[NUM_INPUT_BYTES_OF_SET*2 + 1];
Gah those odd maths again?
> + sprintf(input_buf, "%llX", reqptr->data.data64);
> +
> + int r = dbus_set_property(input_buf);
> +
> + if (r < 0) {
> + fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");
Leak of output_buf?
> + return IPMI_OUT_OF_SPACE;
> + }
> +
> + *data_len = NUM_RETURN_BYTES_OF_SET;
> + // 0x0: return code OK.
> + output_buf[0] = 0x0;
#define?
> + memcpy(response, output_buf, *data_len);
> + free(output_buf);
> }
> else
> {
> + // 0x80: parameter not supported
> + output_buf[0] = 0x80;
#define?
> + memcpy(response, output_buf, *data_len);
> + free(output_buf);
> fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> - return IPMI_CC_PARM_NOT_SUPPORTED;
> + return IPMI_CC_PARM_NOT_SUPPORTED;
> }
>
> return rc;
> }
>
> +
> void register_netfn_chassis_functions()
> {
> printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
> @@ -141,6 +434,9 @@ void register_netfn_chassis_functions()
> printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);
> ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);
>
> + printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS);
> + ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_set_sys_boot_options);
> +
> printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);
> ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);
> }
> diff --git a/chassishandler.h b/chassishandler.h
> index 1a26411..4676667 100644
> --- a/chassishandler.h
> +++ b/chassishandler.h
> @@ -3,12 +3,22 @@
>
> #include <stdint.h>
>
> +// TODO: Petitboot requires 8 bytes of response
> +// however only 5 of them are used. The remaining
> +// 3 bytes are not used in petitboot and the value
> +// of them are all zero.
Just so I'm understanding correctly, you're writing an IPMI daemon here? I'm
not sure you should be noting quirks of petitboot, you should be doing what the
IPMI spec says
> +#define NUM_RETURN_BYTES_OF_GET 8
> +#define NUM_RETURN_BYTES_OF_GET_USED 5
> +#define NUM_RETURN_BYTES_OF_SET 1
> +#define NUM_INPUT_BYTES_OF_SET 5
> +
> // IPMI commands for Chassis net functions.
> enum ipmi_netfn_app_cmds
> {
> // Chassis Control
> IPMI_CMD_CHASSIS_CONTROL = 0x02,
> // Get capability bits
> + IPMI_CMD_SET_SYS_BOOT_OPTIONS = 0x08,
> IPMI_CMD_GET_SYS_BOOT_OPTIONS = 0x09,
> };
>
> @@ -18,6 +28,13 @@ enum ipmi_chassis_return_codes
> IPMI_CC_PARM_NOT_SUPPORTED = 0x80,
> };
>
> +// Generic completion codes,
> +// see IPMI doc section 5.2
> +enum ipmi_generic_return_codes
> +{
> + IPMI_OUT_OF_SPACE = 0xC4,
> +};
> +
> // Various Chassis operations under a single command.
> enum ipmi_chassis_control_cmds : uint8_t
> {
More information about the openbmc
mailing list