[PATCH phosphor-host-ipmid v5] Add get/set ipmid command support with correct DBUS property handling.

[PATCH phosphor-host-ipmid v5] Add get/set ipmid command support with correct DBUS property handling.

[OpenBMC Patches]

1) Add support for IPMI get/set boot option command.
    a) boot options stored in dbus property.
    b) IPMI get boot option command get boot option from the dbus
    property.
    c) IPMI set boot option coomand set boot option to the dbus
    property.
2) Two methods to handle the dbus property set and get:
    a) dbus_set_property()
    b) dbus_get_property()
3) The property is stored as a 10 character strings which representsd 5-byte information.
4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.
5) 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.
6) Error code are properly handled.
7) 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.
8) Fix issues addressed by the community.
    a) change malloc heap to stack local variable.
    b) Fix multi line comment style from "//" to "/**/".
    c) Add defines for return codes.
    d) Add more comments.

https://github.com/openbmc/phosphor-host-ipmid/pull/55

shgoupf (1):
  Add get/set boot option ipmid command support with correct DBUS
    property handling.

 chassishandler.C | 472 ++++++++++++++++++++++++++++++++++++++++++++++---------
 chassishandler.h |  18 +++
 2 files changed, 413 insertions(+), 77 deletions(-)

-- 
2.6.4

[PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.

[OpenBMC Patches]

From: shgoupf <shgoupf@cn.ibm.com>

1) Add support for IPMI get/set boot option command.
    a) boot options stored in dbus property.
    b) IPMI get boot option command get boot option from the dbus
    property.
    c) IPMI set boot option coomand set boot option to the dbus
    property.
2) Two methods to handle the dbus property set and get:
    a) dbus_set_property()
    b) dbus_get_property()
3) The property is stored as a 10 character strings which representsd 5-byte information.
4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.
5) 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.
6) Error code are properly handled.
7) 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.
8) Fix issues addressed by the community.
    a) change malloc heap to stack local variable.
    b) Fix multi line comment style from "//" to "/**/".
    c) Add defines for return codes.
    d) Add more comments.
---
 chassishandler.C | 472 ++++++++++++++++++++++++++++++++++++++++++++++---------
 chassishandler.h |  18 +++
 2 files changed, 413 insertions(+), 77 deletions(-)

diff --git a/chassishandler.C b/chassishandler.C
index 1389db9..8d516de 100644
--- a/chassishandler.C
+++ b/chassishandler.C
@@ -5,9 +5,233 @@
 #include <stdint.h>
 
 // OpenBMC Chassis Manager dbus framework
-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";
+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_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.
+     */
+    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.
+     * TODO: The following code is preserved in the comments so that it can be
+     * resumed after the problem aforementioned is resolved.
+     *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;
+     *}
+     */
+
+    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);
+
+    /*
+     * To represent a hex in string, e.g., "A0A0", which represents two bytes
+     * in the hex, but requires 5 bytes to store it as string, i.e., 4
+     * characters to store the "A0A0" and 1 additional space for "\0".
+     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
+     */
+    memcpy(buf, temp_buf, 2 * NUM_RETURN_BYTES_OF_GET_USED + 1);
+
+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));
 
@@ -15,13 +239,30 @@ struct get_sys_boot_options_t {
     uint8_t parameter;
     uint8_t set;
     uint8_t block;
-}  __attribute__ ((packed));
+}  __attribute__((packed));
 
-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)
+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));
+
+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);
+    printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);
     // Status code.
     ipmi_ret_t rc = IPMI_CC_OK;
     *data_len = 0;
@@ -31,103 +272,176 @@ ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 //------------------------------------------------------------
 // Calls into Chassis Control Dbus object to do the power off
 //------------------------------------------------------------
-int ipmi_chassis_power_control(const char *method)
+int ipmi_chassis_power_control(const char * method)
 {
-	// sd_bus error
-	int rc = 0;
+    // sd_bus error
+    int rc = 0;
 
     // SD Bus error report mechanism.
     sd_bus_error bus_error = SD_BUS_ERROR_NULL;
 
-	// Response from the call. Although there is no response for this call,
-	// obligated to mention this to make compiler happy.
-	sd_bus_message *response = NULL;
-
-	// Gets a hook onto either a SYSTEM or SESSION bus
-	sd_bus *bus_type = ipmid_get_sd_bus_connection();
-
-	rc = sd_bus_call_method(bus_type,        		 // On the System Bus
-							chassis_bus_name,        // Service to contact
-							chassis_object_name,     // Object path 
-							chassis_intf_name,       // Interface name
-							method,      		 // Method to be called
-							&bus_error,      		 // object to return error
-							&response,		 		 // Response buffer if any
-							NULL);			 		 // No input arguments
-	if(rc < 0)
-	{
-		fprintf(stderr,"ERROR initiating Power Off:[%s]\n",bus_error.message);
-	}
-	else
-	{
-		printf("Chassis Power Off initiated successfully\n");
-	}
+    /*
+     * Response from the call. Although there is no response for this call,
+     * obligated to mention this to make compiler happy.
+     */
+    sd_bus_message * response = NULL;
+
+    // Gets a hook onto either a SYSTEM or SESSION bus
+    sd_bus * bus_type = ipmid_get_sd_bus_connection();
+
+    rc = sd_bus_call_method(bus_type,                /* On the System Bus*/
+                            chassis_bus_name,        /* Service to contact*/
+                            chassis_object_name,     /* Object path*/
+                            chassis_intf_name,       /* Interface name*/
+                            method,                  /* Method to be called*/
+                            &bus_error,              /* object to return error*/
+                            &response,               /* Response buffer if any*/
+                            NULL);                   /* No input arguments*/
+
+    if (rc < 0) {
+        fprintf(stderr, "ERROR initiating Power Off:[%s]\n", bus_error.message);
+    } else {
+        printf("Chassis Power Off initiated successfully\n");
+    }
 
     sd_bus_error_free(&bus_error);
     sd_bus_message_unref(response);
 
-	return rc;
+    return rc;
 }
 
 
 //----------------------------------------------------------------------
 // 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_data_len_t data_len, ipmi_context_t context)
+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.
-	int rc = 0;
+    // Error from power off.
+    int rc = 0;
 
-	// No response for this command.
+    // No response for this command.
     *data_len = 0;
 
-	// Catch the actual operaton by peeking into request buffer
-	uint8_t chassis_ctrl_cmd = *(uint8_t *)request;
-	printf("Chassis Control Command: Operation:[0x%X]\n",chassis_ctrl_cmd);
-
-	switch(chassis_ctrl_cmd)
-	{
-		case CMD_POWER_OFF:
-			rc = ipmi_chassis_power_control("powerOff");
-			break;
-		case CMD_HARD_RESET:
-			rc = ipmi_chassis_power_control("reboot");
-			break;
-		default:
-		{
-			fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n",chassis_ctrl_cmd);
-			rc = -1;
-		}
-	}
-
-	return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);
+    // Catch the actual operaton by peeking into request buffer
+    uint8_t chassis_ctrl_cmd = * (uint8_t *)request;
+    printf("Chassis Control Command: Operation:[0x%X]\n", chassis_ctrl_cmd);
+
+    switch (chassis_ctrl_cmd) {
+    case CMD_POWER_OFF:
+        rc = ipmi_chassis_power_control("powerOff");
+        break;
+
+    case CMD_HARD_RESET:
+        rc = ipmi_chassis_power_control("reboot");
+        break;
+
+    default: {
+        fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n", chassis_ctrl_cmd);
+        rc = -1;
+    }
+    }
+
+    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_data_len_t data_len, ipmi_context_t context)
+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;
     *data_len = 0;
 
     printf("IPMI GET_SYS_BOOT_OPTIONS\n");
 
-    get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;
+    get_sys_boot_options_t * reqptr = (get_sys_boot_options_t *) request;
+
+    /*
+     * To represent a hex in string, e.g., "A0A0", which represents two bytes
+     * in the hex, but requires 5 bytes to store it as string, i.e., 4
+     * characters to store the "A0A0" and 1 additional space for "\0".
+     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
+     */
+    char buf[NUM_RETURN_BYTES_OF_GET * 2 + 1] = {0};
 
-    // TODO Return default values to OPAL until dbus interface is available
+    /*
+     * 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 (reqptr->parameter == 5) // Parameter #5
-    {
-        uint8_t buf[] = {0x1,0x5,80,0,0,0,0};
-        *data_len = sizeof(buf);
-        memcpy(response, &buf, *data_len);
+        if (r < 0) {
+            fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
+            return IPMI_OUT_OF_SPACE;
+        }
+
+        uint64_t return_value;
+        sscanf(buf, "%llX", &return_value);
+
+        *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);
+    } else {
+        *data_len = NUM_RETURN_BYTES_OF_GET;
+        // Parameter not supported
+        buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
+        memcpy(response, buf, *data_len);
+        fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
+        return IPMI_CC_PARM_NOT_SUPPORTED;
     }
-    else
-    {
+
+    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[NUM_RETURN_BYTES_OF_SET] = {0};
+
+    /*
+     * 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) {
+        /*
+         * To represent a hex in string, e.g., "A0A0", which represents two bytes
+         * in the hex, but requires 5 bytes to store it as string, i.e., 4
+         * characters to store the "A0A0" and 1 additional space for "\0".
+         * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
+         */
+        char input_buf[NUM_INPUT_BYTES_OF_SET * 2 + 1];
+        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");
+            return IPMI_OUT_OF_SPACE;
+        }
+
+        *data_len = NUM_RETURN_BYTES_OF_SET;
+        // Return code OK.
+        output_buf[0] = IPMI_OK;
+        memcpy(response, output_buf, *data_len);
+    } else {
+        // Parameter not supported
+        output_buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
+        memcpy(response, output_buf, *data_len);
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
-        return IPMI_CC_PARM_NOT_SUPPORTED;        
+        return IPMI_CC_PARM_NOT_SUPPORTED;
     }
 
     return rc;
@@ -135,12 +449,16 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 
 void register_netfn_chassis_functions()
 {
-    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
+    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
     ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);
 
-    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);
+    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_CHASSIS_CONTROL);
+    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..a310741 100644
--- a/chassishandler.h
+++ b/chassishandler.h
@@ -3,21 +3,39 @@
 
 #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.
+#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,
 };
 
 // Command specific completion codes
 enum ipmi_chassis_return_codes
 {
+    IPMI_OK = 0x0,
     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
 {
-- 
2.6.4

Re: [PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.

[Stewart Smith]

OpenBMC Patches <openbmc-patches@stwcx.xyz> writes:
> From: shgoupf <shgoupf@cn.ibm.com>
>
> 1) Add support for IPMI get/set boot option command.
>     a) boot options stored in dbus property.
>     b) IPMI get boot option command get boot option from the dbus
>     property.
>     c) IPMI set boot option coomand set boot option to the dbus
>     property.
> 2) Two methods to handle the dbus property set and get:
>     a) dbus_set_property()
>     b) dbus_get_property()
> 3) The property is stored as a 10 character strings which representsd 5-byte information.
> 4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.
> 5) 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.
> 6) Error code are properly handled.
> 7) 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.
> 8) Fix issues addressed by the community.
>     a) change malloc heap to stack local variable.
>     b) Fix multi line comment style from "//" to "/**/".
>     c) Add defines for return codes.
>     d) Add more comments.

A commit message should describe the change/feature being implemented
and provide additional explanation, this reads like the story of how you
got to this point rather than describing what's going on.

> diff --git a/chassishandler.C b/chassishandler.C
> index 1389db9..8d516de 100644
> --- a/chassishandler.C
> +++ b/chassishandler.C
> @@ -5,9 +5,233 @@
<snip>
> -    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);
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
> +            return IPMI_OUT_OF_SPACE;
> +        }
> +
> +        uint64_t return_value;
> +        sscanf(buf, "%llX", &return_value);

should this be PRIx64 ?

> +
> +        *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);
> +    } else {
> +        *data_len = NUM_RETURN_BYTES_OF_GET;
> +        // Parameter not supported
> +        buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
> +        memcpy(response, buf, *data_len);
> +        fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> +        return IPMI_CC_PARM_NOT_SUPPORTED;
>      }
> -    else
> -    {
> +
> +    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);

these look like debugging printouts? Is there any plan to move to
something better than printing to stdout?

> +
> +    set_sys_boot_options_t * reqptr = (set_sys_boot_options_t *) request;
> +
> +    char output_buf[NUM_RETURN_BYTES_OF_SET] = {0};
> +
> +    /*
> +     * 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) {
> +        /*
> +         * To represent a hex in string, e.g., "A0A0", which represents two bytes
> +         * in the hex, but requires 5 bytes to store it as string, i.e., 4
> +         * characters to store the "A0A0" and 1 additional space for "\0".
> +         * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
> +         */
> +        char input_buf[NUM_INPUT_BYTES_OF_SET * 2 + 1];
> +        sprintf(input_buf, "%llX", reqptr->data.data64);

snprintf is better as it can be more obviously correct, or rather, more
obvious to see that it's not an outright buffer overflow.

You also want to use C99 PRIx64 as otherwise this is incorrect on some
platforms and will emit a warning.

> +
> +        int r = dbus_set_property(input_buf);
> +
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");
> +            return IPMI_OUT_OF_SPACE;
> +        }
> +
> +        *data_len = NUM_RETURN_BYTES_OF_SET;
> +        // Return code OK.
> +        output_buf[0] = IPMI_OK;
> +        memcpy(response, output_buf, *data_len);
> +    } else {
> +        // Parameter not supported
> +        output_buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
> +        memcpy(response, output_buf, *data_len);
>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> -        return IPMI_CC_PARM_NOT_SUPPORTED;        
> +        return IPMI_CC_PARM_NOT_SUPPORTED;

Keep whitespace fixes in a separate patch.

>      }
>  
>      return rc;
> @@ -135,12 +449,16 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  
>  void register_netfn_chassis_functions()
>  {
> -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS,
>  IPMI_CMD_WILDCARD);

same here, separate patch for pure whitespace fix.

>      ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);
>  
> -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);
> +    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_CHASSIS_CONTROL);
> +    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);

and teh same for these.

>  }
> +
> diff --git a/chassishandler.h b/chassishandler.h
> index 1a26411..a310741 100644
> --- a/chassishandler.h
> +++ b/chassishandler.h
> @@ -3,21 +3,39 @@
>  
>  #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.

where? why? why will it never change?
-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.

[Cyril Bur]

On Wed,  6 Jan 2016 02:30:29 -0600
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

Hi Peng,

Thanks for following my recommendations, much appreciated.

Ultimately we have a common goal which is to have the best possible code go
into the project, lets work together on this.



> From: shgoupf <shgoupf@cn.ibm.com>
> 
> 1) Add support for IPMI get/set boot option command.
>     a) boot options stored in dbus property.
>     b) IPMI get boot option command get boot option from the dbus
>     property.
>     c) IPMI set boot option coomand set boot option to the dbus
>     property.
> 2) Two methods to handle the dbus property set and get:
>     a) dbus_set_property()
>     b) dbus_get_property()
> 3) The property is stored as a 10 character strings which representsd 5-byte information.
> 4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.
> 5) 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.
> 6) Error code are properly handled.
> 7) 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.
> 8) Fix issues addressed by the community.
>     a) change malloc heap to stack local variable.
>     b) Fix multi line comment style from "//" to "/**/".
>     c) Add defines for return codes.
>     d) Add more comments.
> ---
>  chassishandler.C | 472 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  chassishandler.h |  18 +++
>  2 files changed, 413 insertions(+), 77 deletions(-)
> 
> diff --git a/chassishandler.C b/chassishandler.C
> index 1389db9..8d516de 100644
> --- a/chassishandler.C
> +++ b/chassishandler.C
> @@ -5,9 +5,233 @@
>  #include <stdint.h>
>  
>  // OpenBMC Chassis Manager dbus framework
> -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";
> +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_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;

Still strange spacing with the * character

> +    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.
> +     */
> +    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);
> +

Still unsure about these interfaces, are you commenting them in the docs repo?


> +    /*
> +     * 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.
> +     * TODO: The following code is preserved in the comments so that it can be
> +     * resumed after the problem aforementioned is resolved.
> +     *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;
> +     *}
> +     */

This still worries me a lot, I've been looking through sd_bus_message_read(),
feel free to take a look,
https://fossies.org/dox/systemd-228/dir_2d204f9a98721f367282e89b81571312.html.

If it returns a negative value there is absolutely no guarantee that it has
correctly set the parameters you passed, it may not have at all depending on at
which point it detects the error. This means you might be reading from an
invalid pointer later on.

I know you say it works, I'm concerned that you might be getting quite lucky
and that it only appears to work.

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

More strange spacing around the *

> +    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);
> +
> +    /*
> +     * To represent a hex in string, e.g., "A0A0", which represents two bytes
> +     * in the hex, but requires 5 bytes to store it as string, i.e., 4
> +     * characters to store the "A0A0" and 1 additional space for "\0".
> +     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
> +     */
> +    memcpy(buf, temp_buf, 2 * NUM_RETURN_BYTES_OF_GET_USED + 1);
> +
> +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 */

Still unsure about these interfaces, are you commenting them in the docs repo?

> +
> +    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));
>  
> @@ -15,13 +239,30 @@ struct get_sys_boot_options_t {
>      uint8_t parameter;
>      uint8_t set;
>      uint8_t block;
> -}  __attribute__ ((packed));
> +}  __attribute__((packed));
>  
> -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)
> +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));
> +
> +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);
> +    printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);
>      // Status code.
>      ipmi_ret_t rc = IPMI_CC_OK;
>      *data_len = 0;
> @@ -31,103 +272,176 @@ ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  //------------------------------------------------------------
>  // Calls into Chassis Control Dbus object to do the power off
>  //------------------------------------------------------------
> -int ipmi_chassis_power_control(const char *method)
> +int ipmi_chassis_power_control(const char * method)
>  {
> -	// sd_bus error
> -	int rc = 0;
> +    // sd_bus error
> +    int rc = 0;
>  
>      // SD Bus error report mechanism.
>      sd_bus_error bus_error = SD_BUS_ERROR_NULL;
>  
> -	// Response from the call. Although there is no response for this call,
> -	// obligated to mention this to make compiler happy.
> -	sd_bus_message *response = NULL;
> -
> -	// Gets a hook onto either a SYSTEM or SESSION bus
> -	sd_bus *bus_type = ipmid_get_sd_bus_connection();
> -
> -	rc = sd_bus_call_method(bus_type,        		 // On the System Bus
> -							chassis_bus_name,        // Service to contact
> -							chassis_object_name,     // Object path 
> -							chassis_intf_name,       // Interface name
> -							method,      		 // Method to be called
> -							&bus_error,      		 // object to return error
> -							&response,		 		 // Response buffer if any
> -							NULL);			 		 // No input arguments
> -	if(rc < 0)
> -	{
> -		fprintf(stderr,"ERROR initiating Power Off:[%s]\n",bus_error.message);
> -	}
> -	else
> -	{
> -		printf("Chassis Power Off initiated successfully\n");
> -	}
> +    /*
> +     * Response from the call. Although there is no response for this call,
> +     * obligated to mention this to make compiler happy.
> +     */
> +    sd_bus_message * response = NULL;
> +
> +    // Gets a hook onto either a SYSTEM or SESSION bus
> +    sd_bus * bus_type = ipmid_get_sd_bus_connection();
> +
> +    rc = sd_bus_call_method(bus_type,                /* On the System Bus*/
> +                            chassis_bus_name,        /* Service to contact*/
> +                            chassis_object_name,     /* Object path*/
> +                            chassis_intf_name,       /* Interface name*/
> +                            method,                  /* Method to be called*/
> +                            &bus_error,              /* object to return error*/
> +                            &response,               /* Response buffer if any*/
> +                            NULL);                   /* No input arguments*/
> +
> +    if (rc < 0) {
> +        fprintf(stderr, "ERROR initiating Power Off:[%s]\n", bus_error.message);
> +    } else {
> +        printf("Chassis Power Off initiated successfully\n");
> +    }
>  
>      sd_bus_error_free(&bus_error);
>      sd_bus_message_unref(response);
>  
> -	return rc;
> +    return rc;
>  }
>  
>  
>  //----------------------------------------------------------------------
>  // 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_data_len_t data_len, ipmi_context_t context)
> +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.
> -	int rc = 0;
> +    // Error from power off.
> +    int rc = 0;
>  
> -	// No response for this command.
> +    // No response for this command.
>      *data_len = 0;
>  
> -	// Catch the actual operaton by peeking into request buffer
> -	uint8_t chassis_ctrl_cmd = *(uint8_t *)request;
> -	printf("Chassis Control Command: Operation:[0x%X]\n",chassis_ctrl_cmd);
> -
> -	switch(chassis_ctrl_cmd)
> -	{
> -		case CMD_POWER_OFF:
> -			rc = ipmi_chassis_power_control("powerOff");
> -			break;
> -		case CMD_HARD_RESET:
> -			rc = ipmi_chassis_power_control("reboot");
> -			break;
> -		default:
> -		{
> -			fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n",chassis_ctrl_cmd);
> -			rc = -1;
> -		}
> -	}
> -
> -	return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);
> +    // Catch the actual operaton by peeking into request buffer
> +    uint8_t chassis_ctrl_cmd = * (uint8_t *)request;
> +    printf("Chassis Control Command: Operation:[0x%X]\n", chassis_ctrl_cmd);
> +
> +    switch (chassis_ctrl_cmd) {
> +    case CMD_POWER_OFF:
> +        rc = ipmi_chassis_power_control("powerOff");
> +        break;
> +
> +    case CMD_HARD_RESET:
> +        rc = ipmi_chassis_power_control("reboot");
> +        break;
> +
> +    default: {
> +        fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n", chassis_ctrl_cmd);
> +        rc = -1;
> +    }
> +    }
> +
> +    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_data_len_t data_len, ipmi_context_t context)
> +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;
>      *data_len = 0;
>  
>      printf("IPMI GET_SYS_BOOT_OPTIONS\n");
>  
> -    get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;
> +    get_sys_boot_options_t * reqptr = (get_sys_boot_options_t *) request;
> +
> +    /*
> +     * To represent a hex in string, e.g., "A0A0", which represents two bytes
> +     * in the hex, but requires 5 bytes to store it as string, i.e., 4
> +     * characters to store the "A0A0" and 1 additional space for "\0".
> +     * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
> +     */
> +    char buf[NUM_RETURN_BYTES_OF_GET * 2 + 1] = {0};

I still don't follow why these odd #define numbers, why don't you put the * 2 +
1 inside the #define?

Or, really we should be discussing why numbers are being passed around as
strings.
>  
> -    // TODO Return default values to OPAL until dbus interface is available
> +    /*
> +     * 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 (reqptr->parameter == 5) // Parameter #5
> -    {
> -        uint8_t buf[] = {0x1,0x5,80,0,0,0,0};
> -        *data_len = sizeof(buf);
> -        memcpy(response, &buf, *data_len);
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
> +            return IPMI_OUT_OF_SPACE;
> +        }
> +
> +        uint64_t return_value;
> +        sscanf(buf, "%llX", &return_value);

Why can't we a) be passing numbers around as numbers b) use the correct tool
for the job.

> +
> +        *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);
> +    } else {
> +        *data_len = NUM_RETURN_BYTES_OF_GET;
> +        // Parameter not supported
> +        buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
> +        memcpy(response, buf, *data_len);

... previous comments, there really is no need for the memcpy

> +        fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> +        return IPMI_CC_PARM_NOT_SUPPORTED;
>      }
> -    else
> -    {
> +
> +    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[NUM_RETURN_BYTES_OF_SET] = {0};
> +
> +    /*
> +     * 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) {
> +        /*
> +         * To represent a hex in string, e.g., "A0A0", which represents two bytes
> +         * in the hex, but requires 5 bytes to store it as string, i.e., 4
> +         * characters to store the "A0A0" and 1 additional space for "\0".
> +         * Thereby we have 2 * <number of bytes> + 1 for the string buffer.
> +         */
> +        char input_buf[NUM_INPUT_BYTES_OF_SET * 2 + 1];
> +        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");
> +            return IPMI_OUT_OF_SPACE;
> +        }
> +
> +        *data_len = NUM_RETURN_BYTES_OF_SET;
> +        // Return code OK.
> +        output_buf[0] = IPMI_OK;
> +        memcpy(response, output_buf, *data_len);
> +    } else {
> +        // Parameter not supported
> +        output_buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;
> +        memcpy(response, output_buf, *data_len);

... previous comments, there really is no need for the memcpy

>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
> -        return IPMI_CC_PARM_NOT_SUPPORTED;        
> +        return IPMI_CC_PARM_NOT_SUPPORTED;
>      }
>  
>      return rc;
> @@ -135,12 +449,16 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  
>  void register_netfn_chassis_functions()
>  {
> -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
> +    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
>      ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);
>  
> -    printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);
> +    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_CHASSIS_CONTROL);
> +    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..a310741 100644
> --- a/chassishandler.h
> +++ b/chassishandler.h
> @@ -3,21 +3,39 @@
>  
>  #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.
> +#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,
>  };
>  
>  // Command specific completion codes
>  enum ipmi_chassis_return_codes
>  {
> +    IPMI_OK = 0x0,
>      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
>  {

Re: [PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.

[Peng Fei BG Gou]

[-- Attachment #1: Type: text/html, Size: 12843 bytes --]

Re: [PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.

[Peng Fei BG Gou]

[-- Attachment #1: Type: text/html, Size: 44703 bytes --]

[PATCH phosphor-host-ipmid v5] Add get/set ipmid command support with correct DBUS property handling.

[OpenBMC Patches]

From: Chris Austen <austenc@us.ibm.com>

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 represents
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.
---
 chassishandler.C | 290 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 chassishandler.h |   8 ++
 2 files changed, 289 insertions(+), 9 deletions(-)

diff --git a/chassishandler.C b/chassishandler.C
index 56b8375..35b5086 100644
--- a/chassishandler.C
+++ b/chassishandler.C
@@ -9,6 +9,217 @@ 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";
+
+char* uint8_to_char(uint8_t *a, size_t size)
+{
+    char* buffer;
+    int i;
+
+    buffer = (char*)malloc(size * 2 + 1);
+    if (!buffer)
+        return NULL;
+
+    buffer[size * 2] = 0;
+    for (i = 0; i < size; i++) {
+        uint8_t msb = (a[i] >> 4) & 0xF;
+        uint8_t lsb = a[i] & 0xF;
+        buffer[2*i] = msb > 9 ? msb + 'A' - 10 : msb + '0';
+        buffer[2*i + 1] = lsb > 9 ? lsb + 'A' - 10 : lsb + '0';
+    }
+
+    return buffer;
+}
+
+uint8_t* char_to_uint8(char *a, size_t size)
+{
+    uint8_t* buffer;
+    int i;
+
+    buffer = (uint8_t*)malloc(size);
+    if (!buffer)
+        return NULL;
+
+    for (i = 0; i < size; i++) {
+        uint8_t msb = (uint8_t)(a[2*i] > '9' ? a[2*i] - 'A' + 10 : a[2*i] - '0');
+        uint8_t lsb = (uint8_t)(a[2*i+1] > '9' ? a[2*i+1] - 'A' + 10 : a[2*i+1] - '0');
+        buffer[i] = ((msb << 4) | (lsb & 0xF)) & 0xFF;
+    }
+
+    return buffer;
+}
+
+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;
+    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.
+    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
+    r = sd_bus_message_read(m, "a{sas}", 1, &temp_buf);
+    buf_size = strlen(temp_buf) + 1;
+    printf("IPMID connection name: %s\n", temp_buf);
+    *buf = (char*)malloc(buf_size);
+    memcpy(*buf, temp_buf, buf_size);
+
+finish:
+    sd_bus_error_free(&error);
+    sd_bus_message_unref(m);
+    sd_bus_unref(bus);
+
+    return r;
+}
+
+// TODO: object mapper should be used instead of hard-coding.
+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;
+
+    // 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;
+    }
+
+    object_mapper_get_connection(&connection, settings_object_name);
+    printf("connection: %s\n", connection);
+    // 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);
+
+finish:
+    sd_bus_error_free(&error);
+    sd_bus_message_unref(m);
+    sd_bus_unref(bus);
+    free(connection);
+
+    return r;
+}
+
+// TODO: object mapper should be used instead of hard-coding.
+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;
+
+    // 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;
+    }
+
+    object_mapper_get_connection(&connection, settings_object_name);
+    printf("connection: %s\n", connection);
+    // 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,6 +228,15 @@ struct get_sys_boot_options_t {
     uint8_t block;
 }  __attribute__ ((packed));
 
+struct set_sys_boot_options_t {
+    uint8_t parameter;
+    uint8_t data1;
+    uint8_t data2;
+    uint8_t data3;
+    uint8_t data4;
+    uint8_t data5;
+}  __attribute__ ((packed));
+
 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)
@@ -31,7 +251,7 @@ ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 //------------------------------------------------------------
 // Calls into Chassis Control Dbus object to do the power off
 //------------------------------------------------------------
-int ipmi_chassis_power_off()
+int ipmi_chassis_power_control(const char *method)
 {
 	// sd_bus error
 	int rc = 0;
@@ -50,7 +270,7 @@ int ipmi_chassis_power_off()
 							chassis_bus_name,        // Service to contact
 							chassis_object_name,     // Object path 
 							chassis_intf_name,       // Interface name
-							"powerOff",      		 // Method to be called
+							method,      		 // Method to be called
 							&bus_error,      		 // object to return error
 							&response,		 		 // Response buffer if any
 							NULL);			 		 // No input arguments
@@ -69,6 +289,7 @@ int ipmi_chassis_power_off()
 	return rc;
 }
 
+
 //----------------------------------------------------------------------
 // Chassis Control commands
 //----------------------------------------------------------------------
@@ -89,11 +310,11 @@ ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 	switch(chassis_ctrl_cmd)
 	{
 		case CMD_POWER_OFF:
+			rc = ipmi_chassis_power_control("powerOff");
+			break;
 		case CMD_HARD_RESET:
-		{
-			rc = ipmi_chassis_power_off();
+			rc = ipmi_chassis_power_control("reboot");
 			break;
-		}
 		default:
 		{
 			fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n",chassis_ctrl_cmd);
@@ -115,16 +336,63 @@ 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);
+
+    if (reqptr->parameter == 5) // Parameter #5
+    {
+        dbus_get_property(buf);
+        uint8_t* return_value = char_to_uint8(buf, NUM_RETURN_BYTES_OF_GET_USED);
+        *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, return_value, *data_len);
+        free(buf);
+        free(return_value);
+    }
+    else
+    {
+        *data_len = NUM_RETURN_BYTES_OF_GET;
+        // 0x80: parameter not supported
+        buf[0] = 0x80;
+        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 (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 = uint8_to_char((uint8_t*)(&(reqptr->data1)), NUM_INPUT_BYTES_OF_SET);
+        dbus_set_property(input_buf);
+        *data_len = NUM_RETURN_BYTES_OF_SET;
+        // 0x0: return code OK.
+        output_buf[0] = 0x0;
+        memcpy(response, output_buf, *data_len);
+        free(output_buf);
+        free(input_buf);
     }
     else
     {
+        // 0x80: parameter not supported
+        output_buf[0] = 0x80;
+        memcpy(response, output_buf, *data_len);
+        free(output_buf);
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
         return IPMI_CC_PARM_NOT_SUPPORTED;        
     }
@@ -132,6 +400,7 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
     return rc;
 }
 
+
 void register_netfn_chassis_functions()
 {
     printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);
@@ -140,6 +409,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..0164ce1 100644
--- a/chassishandler.h
+++ b/chassishandler.h
@@ -3,12 +3,20 @@
 
 #include <stdint.h>
 
+// TODO: Petitboot requires 8 bytes of response
+// however only 5 of them are used.
+#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,
 };
 
-- 
2.6.3