<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Hey Cyril,</div>
<div dir="ltr" > </div>
<div dir="ltr" >Thanks for your detailed review on this patch.</div>
<div dir="ltr" > </div>
<div dir="ltr" >Just want to answer (most of) your questions in batch:</div>
<div dir="ltr" >1) This patch intended to add the functionality of ipmi set/get boot option, with dbus property and object mapper used, so I believe all of the contents in this patch should be considered as a whole. We shouldn't split them into multiple patches.</div>
<div dir="ltr" >2) atoi is an alternative, but I don't see there is any reason why to use it since sscanf/sprintf works fine for now. I don't believe there is too much performance concern there. </div>
<div dir="ltr" >3) For those sd_bus method call with signatures ("s", "v", "s{ass}", etc.), I believe the current way I use it is valid since I have tested them already.</div>
<div dir="ltr" >4) For the commented out error handling, I believe it's just a work around, but it did works according to my test. So I believe it's OK for this patch to have it and we should revisit it later.</div>
<div dir="ltr" >5) For the way how return code is handled, I believe it is working fine.</div>
<div dir="ltr" >6) For the coding style stuff, such as spaces between function names and the bracket, I don't think it's a major issue. Maybe it's a minor issue that they are not consistent with some piece of code elsewhere. It can be fixed with my auto style tool very easily. </div>
<div dir="ltr" > </div>
<div dir="ltr" >In a sum, thanks for your effort on reviewing this, but I believe most of your current concerns either are invalid or are only minor issues.</div>
<div dir="ltr" > </div>
<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >GOU, Peng Fei (苟鹏飞), Ph.D.</div>
<div dir="ltr" >OpenPower Team.</div>
<div dir="ltr" >+86-21-609-28631</div></div></div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<blockquote data-history-content-modified="1" dir="ltr" style="border-left:solid #aaaaaa 2px; margin-left:5px; padding-left:5px; direction:ltr; margin-right:0px" >----- Original message -----<br>From: Cyril Bur <cyrilbur@gmail.com><br>To: OpenBMC Patches <openbmc-patches@stwcx.xyz>, Peng Fei BG Gou/China/IBM@IBMCN<br>Cc: openbmc@lists.ozlabs.org<br>Subject: Re: [PATCH phosphor-host-ipmid v4] Add get/set boot option ipmid command support with correct DBUS property handling.<br>Date: Wed, Jan 6, 2016 1:10 PM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Mon, 4 Jan 2016 19:10:26 -0600<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>> From: shgoupf <shgoupf@cn.ibm.com><br>><br><br>Hi again,<br><br>> 1) Add support for IPMI get/set boot option command.<br>> a) boot options stored in dbus property.<br>> b) IPMI get boot option command get boot option from the dbus<br>> property.<br>> c) IPMI set boot option coomand set boot option to the dbus<br>> property.<br>> 2) Two methods to handle the dbus property set and get:<br>> a) dbus_set_property()<br>> b) dbus_get_property()<br>> 3) The property is stored as a 10 character strings which representsd 5-byte information.<br>> 4) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.<br>> 5) Get service name via object mapper<br>> a) The connection name is got via objectmapper.<br>> b) The method used to get the connection name is object_mapper_get_connection().<br>> c) dbus_get_property/dbus_set_property will get the connection name via the above method instead of hard coding.<br>> 6) Error code are properly handled.<br>> 7) Use sprinf/sscanf for int/string conversion.<br>> a) Instead of reinventing the wheel by defining methods converting int to string, use sprintf/sscanf should be a more clean and robust way.<br><br>Plus 1 for not reinventing the wheel but perhaps consider using the best tool<br>for the job, sscanf can and does do what you want however atoi, atol, atoll,<br>strtol, strtoll, strtoq functions are all designed for this exact purpose. They<br>will perform much better than sscanf, also the str* versions give great control<br>and error detection capabilities.<br><br><br>> 8) Fix issues addressed by the community.<br>> a) change malloc heap to stack local variable.<br>> b) Fix multi line comment style from "//" to "/**/".<br>> c) Add defines for return codes.<br>> d) Add more comments.<br><br>It looks like you're adding more and more into this same patch. A single patch<br>should be one clearly defined piece of work. So you're implementing some new<br>functionality, that should be its own patch. You're also adding some error<br>handling paths - also, its own patch. You've changed some whitespace (some of<br>which was a bad thing) but the good whitespace changes should also be their own<br>patch.<br><br>> chassishandler.C | 492 +++++++++++++++++++++++++++++++++++++++++++++----------<br>> chassishandler.h | 18 ++<br>> 2 files changed, 423 insertions(+), 87 deletions(-)<br>><br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index 1389db9..4d941e8 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -5,11 +5,235 @@<br>> #include <stdint.h><br>> <br>> // OpenBMC Chassis Manager dbus framework<br>> -const char *chassis_bus_name = "org.openbmc.control.Chassis";<br>> -const char *chassis_object_name = "/org/openbmc/control/chassis0";<br>> -const char *chassis_intf_name = "org.openbmc.control.Chassis";<br>> +const char * chassis_bus_name = "org.openbmc.control.Chassis";<br>> +const char * chassis_object_name = "/org/openbmc/control/chassis0";<br>> +const char * chassis_intf_name = "org.openbmc.control.Chassis";<br>> <br><br>Not necessary, if anything you should remove the double space but what you've<br>done doesn't look good.<br><br>> -void register_netfn_chassis_functions() __attribute__((constructor));<br>> +// Host settings in dbus<br>> +// Service name should be referenced by connection name got via object mapper<br>> +const char * settings_object_name = "/org/openbmc/settings/host0";<br>> +const char * settings_intf_name = "org.freedesktop.DBus.Properties";<br>> +<br>> +const char * objmapper_service_name = "org.openbmc.objectmapper";<br>> +const char * objmapper_object_name = "/org/openbmc/objectmapper/objectmapper";<br>> +const char * objmapper_intf_name = "org.openbmc.objectmapper.ObjectMapper";<br>> +<br><br>We probably want to try to keep a consistent style. I looked at ipmid.C and<br>that file isn't consistent but it seems to be mostly "type *name;" and that's<br>the most common way everywhere else with C code.<br><br>> +int object_mapper_get_connection (char ** buf, const char * obj_path)<br>> +{<br>> + sd_bus_error error = SD_BUS_ERROR_NULL;<br>> + sd_bus_message * m = NULL;<br>> + sd_bus * bus = NULL;<br>> + char * temp_buf = NULL, *intf = NULL;<br>> + size_t buf_size = 0;<br>> + int r;<br>> +<br>> + // Open the system bus where most system services are provided.<br>> + r = sd_bus_open_system (&bus);<br>> +<br><br>As an added stylistic note, again for consistency, ipmid.C, you should avoid<br>having a space between the function name and the parameters<br><br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> + goto finish;<br>> + }<br>> +<br>> + /*<br>> + * Bus, service, object path, interface and method are provided to call<br>> + * the method.<br>> + * Signatures and input arguments are provided by the arguments at the<br>> + * end.<br>> + */<br>> + r = sd_bus_call_method (bus,<br>> + objmapper_service_name, /* service to contact */<br>> + objmapper_object_name, /* object path */<br>> + objmapper_intf_name, /* interface name */<br>> + "GetObject", /* method name */<br>> + &error, /* object to return error in */<br>> + &m, /* return message on success */<br>> + "s", /* input signature */<br>> + obj_path /* first argument */<br>> + );<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> + goto finish;<br>> + }<br>> +<br>> + // Get the key, aka, the connection name<br>> + sd_bus_message_read (m, "a{sas}", 1, &temp_buf, 1, &intf);<br>> +<br><br>This seems very roundabout, could you explain this method call. What are you<br>expecting to receive? From the looks of what you're using it for wouldn't it<br>make more sense to just sd_bus_message_read a "s"?<br><br>> + /*<br>> + * TODO: check the return code. Currently for no reason the message<br>> + * parsing of object mapper is always complaining about<br>> + * "Device or resource busy", but the result seems OK for now. Need<br>> + * further checks.<br><br>If sd_bus_message_read returns a negative value, something definitely went<br>wrong, I highly highly doubt you can use the result if it turns a negative.<br><br>> + * TODO: The following code is preserved in the comments so that it can be<br>> + * resumed after the problem aforementioned is resolved.<br>> + *r = sd_bus_message_read(m, "a{sas}", 1, &temp_buf, 1, &intf);<br>> + *if (r < 0) {<br>> + * fprintf(stderr, "Failed to parse response message: %s\n", strerror(-r));<br>> + * goto finish;<br>> + *}<br><br>The code is in git, it is saved there, delete it now and when someone wants to<br>add it back they can look in the git history for what was deleted. Having said<br>that, the error check should be there, if you're hitting it, you should figure<br>out why.<br><br>> + */<br>> +<br>> + buf_size = strlen (temp_buf) + 1;<br>> + printf ("IPMID connection name: %s\n", temp_buf);<br>> + *buf = (char *)malloc (buf_size);<br>> +<br>> + if (*buf == NULL) {<br>> + fprintf (stderr, "Malloc failed for get_sys_boot_options");<br>> + r = -1;<br>> + goto finish;<br>> + }<br>> +<br><br>I don't mind using malloc but these are strings no? Couldn't strndup be used,<br>might make life easier? Not a big concern.<br><br>> + memcpy (*buf, temp_buf, buf_size);<br>> +<br>> +finish:<br>> + sd_bus_error_free (&error);<br>> + sd_bus_message_unref (m);<br>> + sd_bus_unref (bus);<br>> +<br><br>Spaces between name and params<br><br>> + return r;<br>> +}<br>> +<br>> +int dbus_get_property (char * buf)<br>> +{<br>> + sd_bus_error error = SD_BUS_ERROR_NULL;<br>> + sd_bus_message * m = NULL;<br>> + sd_bus * bus = NULL;<br>> + char * temp_buf = NULL;<br>> + uint8_t * get_value = NULL;<br>> + char * connection = NULL;<br>> + int r, i;<br>> +<br>> + r = object_mapper_get_connection (&connection, settings_object_name);<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to get connection, return value: %d.\n", r);<br>> + goto finish;<br>> + }<br>> +<br>> + printf ("connection: %s\n", connection);<br>> +<br>> + // Open the system bus where most system services are provided.<br>> + r = sd_bus_open_system (&bus);<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> + goto finish;<br>> + }<br>> +<br>> + /*<br>> + * Bus, service, object path, interface and method are provided to call<br>> + * the method.<br>> + * Signatures and input arguments are provided by the arguments at the<br>> + * end.<br>> + */<br>> + r = sd_bus_call_method (bus,<br>> + connection, /* service to contact */<br>> + settings_object_name, /* object path */<br>> + settings_intf_name, /* interface name */<br>> + "Get", /* method name */<br>> + &error, /* object to return error in */<br>> + &m, /* return message on success */<br>> + "ss", /* input signature */<br>> + settings_intf_name, /* first argument */<br>> + "boot_flags"); /* second argument */<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> + goto finish;<br>> + }<br>> +<br>> + /*<br>> + * The output should be parsed exactly the same as the output formatting<br>> + * specified.<br>> + */<br>> + r = sd_bus_message_read (m, "v", "s", &temp_buf);<br>> +<br><br>Why not just read a "s"?<br><br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to parse response message: %s\n", strerror (-r));<br>> + goto finish;<br>> + }<br>> +<br>> + printf ("IPMID boot option property get: {%s}.\n", (char *) temp_buf);<br>> +<br>> + /*<br>> + * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> + * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> + * characters to store the "A0A0" and 1 additional space for "\0".<br>> + * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> + */<br><br>So why don't you have #define NUM_RETURN_BYTES_OF_GET_USED 11 ?<br><br>This question is out of scope of this patch but why are we passing around<br>numbers as strings? dbus handles other types...<br><br>> + memcpy (buf, temp_buf, 2 * NUM_RETURN_BYTES_OF_GET_USED + 1);<br>> +<br>> +finish:<br>> + sd_bus_error_free (&error);<br>> + sd_bus_message_unref (m);<br>> + sd_bus_unref (bus);<br>> + free (connection);<br>> +<br>> + return r;<br>> +}<br>> +<br>> +int dbus_set_property (const char * buf)<br>> +{<br>> + sd_bus_error error = SD_BUS_ERROR_NULL;<br>> + sd_bus_message * m = NULL;<br>> + sd_bus * bus = NULL;<br>> + char * connection = NULL;<br>> + int r;<br>> +<br>> + r = object_mapper_get_connection (&connection, settings_object_name);<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to get connection, return value: %d.\n", r);<br>> + goto finish;<br>> + }<br>> +<br>> + printf ("connection: %s\n", connection);<br>> +<br>> + // Open the system bus where most system services are provided.<br>> + r = sd_bus_open_system (&bus);<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to connect to system bus: %s\n", strerror (-r));<br>> + goto finish;<br>> + }<br>> +<br>> + /*<br>> + * Bus, service, object path, interface and method are provided to call<br>> + * the method.<br>> + * Signatures and input arguments are provided by the arguments at the<br>> + * end.<br>> + */<br>> + r = sd_bus_call_method (bus,<br>> + connection, /* service to contact */<br>> + settings_object_name, /* object path */<br>> + settings_intf_name, /* interface name */<br>> + "Set", /* method name */<br>> + &error, /* object to return error in */<br>> + &m, /* return message on success */<br>> + "ssv", /* input signature */<br>> + settings_intf_name, /* first argument */<br>> + "boot_flags", /* second argument */<br>> + "s", /* third argument */<br>> + buf); /* fourth argument */<br>> +<br><br>Are we documenting these interfaces at all, I don't understand why "ssv" and<br>then a hardcoded "s", why not "sss"?<br><br>> + if (r < 0) {<br>> + fprintf (stderr, "Failed to issue method call: %s\n", error.message);<br>> + goto finish;<br>> + }<br>> +<br>> + printf ("IPMID boot option property set: {%s}.\n", buf);<br>> +<br>> +finish:<br>> + sd_bus_error_free (&error);<br>> + sd_bus_message_unref (m);<br>> + sd_bus_unref (bus);<br>> + free (connection);<br>> +<br>> + return r;<br>> +}<br>> +<br>> +void register_netfn_chassis_functions () __attribute__ ((constructor));<br>> <br>> struct get_sys_boot_options_t {<br>> uint8_t parameter;<br>> @@ -17,11 +241,28 @@ struct get_sys_boot_options_t {<br>> uint8_t block;<br>> } __attribute__ ((packed));<br>> <br>> -ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> - ipmi_request_t request, ipmi_response_t response,<br>> - ipmi_data_len_t data_len, ipmi_context_t context)<br>> +struct set_sys_boot_options_t {<br>> + uint8_t parameter;<br>> + union {<br>> + struct {<br>> + uint8_t d1;<br>> + uint8_t d2;<br>> + uint8_t d3;<br>> + uint8_t d4;<br>> + uint8_t d5;<br>> + uint8_t d6;<br>> + uint8_t d7;<br>> + uint8_t d8;<br>> + } data8;<br>> + uint64_t data64;<br>> + } data;<br>> +} __attribute__ ((packed));<br>> +<br>> +ipmi_ret_t ipmi_chassis_wildcard (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> + ipmi_request_t request, ipmi_response_t response,<br>> + ipmi_data_len_t data_len, ipmi_context_t context)<br>> {<br>> - printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);<br>> + printf ("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n", netfn, cmd);<br><br>Be careful with changes like this, you didn't change this code...<br><br>> // Status code.<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> *data_len = 0;<br>> @@ -31,116 +272,193 @@ ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> //------------------------------------------------------------<br>> // Calls into Chassis Control Dbus object to do the power off<br>> //------------------------------------------------------------<br>> -int ipmi_chassis_power_control(const char *method)<br>> +int ipmi_chassis_power_control (const char * method)<br>> {<br>> - // sd_bus error<br>> - int rc = 0;<br>> + // sd_bus error<br>> + int rc = 0;<br>> <br>> // SD Bus error report mechanism.<br>> sd_bus_error bus_error = SD_BUS_ERROR_NULL;<br>> <br>> - // Response from the call. Although there is no response for this call,<br>> - // obligated to mention this to make compiler happy.<br>> - sd_bus_message *response = NULL;<br>> -<br>> - // Gets a hook onto either a SYSTEM or SESSION bus<br>> - sd_bus *bus_type = ipmid_get_sd_bus_connection();<br>> -<br>> - rc = sd_bus_call_method(bus_type, // On the System Bus<br>> - chassis_bus_name, // Service to contact<br>> - chassis_object_name, // Object path<br>> - chassis_intf_name, // Interface name<br>> - method, // Method to be called<br>> - &bus_error, // object to return error<br>> - &response, // Response buffer if any<br>> - NULL); // No input arguments<br>> - if(rc < 0)<br>> - {<br>> - fprintf(stderr,"ERROR initiating Power Off:[%s]\n",bus_error.message);<br>> - }<br>> - else<br>> - {<br>> - printf("Chassis Power Off initiated successfully\n");<br>> - }<br>> -<br>> - sd_bus_error_free(&bus_error);<br>> - sd_bus_message_unref(response);<br>> -<br>> - return rc;<br>> + /*<br>> + * Response from the call. Although there is no response for this call,<br>> + * obligated to mention this to make compiler happy.<br>> + */<br>> + sd_bus_message * response = NULL;<br>> +<br>> + // Gets a hook onto either a SYSTEM or SESSION bus<br>> + sd_bus * bus_type = ipmid_get_sd_bus_connection ();<br>> +<br>> + rc = sd_bus_call_method (bus_type, /* On the System Bus*/<br>> + chassis_bus_name, /* Service to contact*/<br>> + chassis_object_name, /* Object path*/<br>> + chassis_intf_name, /* Interface name*/<br>> + method, /* Method to be called*/<br>> + &bus_error, /* object to return error*/<br>> + &response, /* Response buffer if any*/<br>> + NULL); /* No input arguments*/<br>> +<br>> + if (rc < 0) {<br>> + fprintf (stderr, "ERROR initiating Power Off:[%s]\n", bus_error.message);<br>> + } else {<br>> + printf ("Chassis Power Off initiated successfully\n");<br>> + }<br>> +<br>> + sd_bus_error_free (&bus_error);<br>> + sd_bus_message_unref (response);<br>> +<br>> + return rc;<br><br>Cleanups are great but please send them as a separate patch, this helps people<br>a lot when reviewing patches, if it is just a cleanup then people can focus on<br>checking that you didn't change behaviour OR if you're sending a functional<br>change patch people can focus on checking that you're achieving what you set<br>out to do.<br><br>> }<br>> <br>> <br>> //----------------------------------------------------------------------<br>> // Chassis Control commands<br>> //----------------------------------------------------------------------<br>> -ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> - ipmi_request_t request, ipmi_response_t response,<br>> - ipmi_data_len_t data_len, ipmi_context_t context)<br>> +ipmi_ret_t ipmi_chassis_control (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> + ipmi_request_t request, ipmi_response_t response,<br>> + ipmi_data_len_t data_len, ipmi_context_t context)<br>> {<br>> - // Error from power off.<br>> - int rc = 0;<br>> + // Error from power off.<br>> + int rc = 0;<br>> <br>> - // No response for this command.<br>> + // No response for this command.<br>> *data_len = 0;<br>> <br>> - // Catch the actual operaton by peeking into request buffer<br>> - uint8_t chassis_ctrl_cmd = *(uint8_t *)request;<br>> - printf("Chassis Control Command: Operation:[0x%X]\n",chassis_ctrl_cmd);<br>> -<br>> - switch(chassis_ctrl_cmd)<br>> - {<br>> - case CMD_POWER_OFF:<br>> - rc = ipmi_chassis_power_control("powerOff");<br>> - break;<br>> - case CMD_HARD_RESET:<br>> - rc = ipmi_chassis_power_control("reboot");<br>> - break;<br>> - default:<br>> - {<br>> - fprintf(stderr, "Invalid Chassis Control command:[0x%X] received\n",chassis_ctrl_cmd);<br>> - rc = -1;<br>> - }<br>> - }<br>> -<br>> - return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);<br>> + // Catch the actual operaton by peeking into request buffer<br>> + uint8_t chassis_ctrl_cmd = * (uint8_t *)request;<br>> + printf ("Chassis Control Command: Operation:[0x%X]\n", chassis_ctrl_cmd);<br>> +<br>> + switch (chassis_ctrl_cmd) {<br>> + case CMD_POWER_OFF:<br>> + rc = ipmi_chassis_power_control ("powerOff");<br>> + break;<br>> +<br>> + case CMD_HARD_RESET:<br>> + rc = ipmi_chassis_power_control ("reboot");<br>> + break;<br>> +<br>> + default: {<br>> + fprintf (stderr, "Invalid Chassis Control command:[0x%X] received\n", chassis_ctrl_cmd);<br>> + rc = -1;<br>> + }<br>> + }<br>> +<br>> + return ((rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);<br>> }<br>> <br>> -ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> - ipmi_request_t request, ipmi_response_t response,<br>> - ipmi_data_len_t data_len, ipmi_context_t context)<br>> +ipmi_ret_t ipmi_chassis_get_sys_boot_options (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> + ipmi_request_t request, ipmi_response_t response,<br>> + ipmi_data_len_t data_len, ipmi_context_t context)<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> *data_len = 0;<br>> <br>> - printf("IPMI GET_SYS_BOOT_OPTIONS\n");<br>> + printf ("IPMI GET_SYS_BOOT_OPTIONS\n");<br>> +<br>> + get_sys_boot_options_t * reqptr = (get_sys_boot_options_t *) request;<br>> +<br>> + /*<br>> + * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> + * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> + * characters to store the "A0A0" and 1 additional space for "\0".<br>> + * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> + */<br>> + char buf[NUM_RETURN_BYTES_OF_GET * 2 + 1] = {0};<br><br>Once again, why not #define NUM_RETURN_BYTES_OF_GET 11<br><br>> <br>> - get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;<br>> + /*<br>> + * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.<br>> + * This is the only parameter used by petitboot.<br>> + */<br>> + if (reqptr->parameter == 5) {<br>> + int r = dbus_get_property (buf);<br>> <br>> - // TODO Return default values to OPAL until dbus interface is available<br>> + if (r < 0) {<br>> + fprintf (stderr, "Dbus get property failed for get_sys_boot_options.\n");<br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> <br>> - if (reqptr->parameter == 5) // Parameter #5<br>> - {<br>> - uint8_t buf[] = {0x1,0x5,80,0,0,0,0};<br>> - *data_len = sizeof(buf);<br>> - memcpy(response, &buf, *data_len);<br>> + uint64_t return_value;<br>> + sscanf (buf, "%llX", &return_value);<br><br>See my comment at the top about the best tool for the job.<br><br>> +<br>> + *data_len = NUM_RETURN_BYTES_OF_GET;<br>> + /*<br>> + * TODO: last 3 bytes<br>> + *(NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless<br>> + */<br>> + memcpy (response, (uint8_t *) (&return_value), *data_len);<br><br>So this interests me, does whoever is going to be reading response know to read<br>it as a uint64_t on success but as an array of chars on failure, also, rather<br>than #defineing, wouldn't:<br><br>*data_len = sizeof(return_value);<br><br>be better?<br><br><br>I've done some looking around in ipmid.C and I don't think what you've done<br>will work,<br><a href="https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.C#L276" target="_blank" >https://github.com/openbmc/phosphor-host-ipmid/blob/master/ipmid.C#L276</a><br>appears to use the first byte of response as the completion code but you've<br>just written whatever the first byte of your uint64_t is into response[0]...<br><br>Also, 28.13 in the IPMI spec doesn't seem to match up with whats going on here,<br>am I missing something?<br><br>> + } else {<br>> + *data_len = NUM_RETURN_BYTES_OF_GET;<br>> + // Parameter not supported<br>> + buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;<br>> + memcpy (response, buf, *data_len);<br>> + fprintf (stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> + return IPMI_CC_PARM_NOT_SUPPORTED;<br>> }<br>> - else<br>> - {<br>> - fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> - return IPMI_CC_PARM_NOT_SUPPORTED; <br>> +<br>> + return rc;<br>> +}<br>> +<br>> +ipmi_ret_t ipmi_chassis_set_sys_boot_options (ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> + ipmi_request_t request, ipmi_response_t response,<br>> + ipmi_data_len_t data_len, ipmi_context_t context)<br>> +{<br>> + ipmi_ret_t rc = IPMI_CC_OK;<br>> +<br>> + printf ("IPMI SET_SYS_BOOT_OPTIONS\n");<br>> + printf ("IPMID set command required return bytes: %i\n", *data_len);<br>> +<br>> + set_sys_boot_options_t * reqptr = (set_sys_boot_options_t *) request;<br>> +<br>> + char output_buf[NUM_RETURN_BYTES_OF_SET] = {0};<br>> +<br>> + /*<br>> + * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.<br>> + * This is the only parameter used by petitboot.<br>> + */<br>> + if (reqptr->parameter == 5) {<br>> + /*<br>> + * To represent a hex in string, e.g., "A0A0", which represents two bytes<br>> + * in the hex, but requires 5 bytes to store it as string, i.e., 4<br>> + * characters to store the "A0A0" and 1 additional space for "\0".<br>> + * Thereby we have 2 * <number of bytes> + 1 for the string buffer.<br>> + */<br>> + char input_buf[NUM_INPUT_BYTES_OF_SET * 2 + 1];<br>> + sprintf (input_buf, "%llX", reqptr->data.data64);<br>> +<br>> + int r = dbus_set_property (input_buf);<br>> +<br>> + if (r < 0) {<br>> + fprintf (stderr, "Dbus set property failed for set_sys_boot_options.\n");<br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> +<br>> + *data_len = NUM_RETURN_BYTES_OF_SET;<br>> + // Return code OK.<br>> + output_buf[0] = IPMI_OK;<br>> + memcpy (response, output_buf, *data_len);<br>> + } else {<br>> + // Parameter not supported<br>> + output_buf[0] = IPMI_CC_PARM_NOT_SUPPORTED;<br>> + memcpy (response, output_buf, *data_len);<br>> + fprintf (stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> + return IPMI_CC_PARM_NOT_SUPPORTED;<br><br>As far as I can see you're simply using output_buf to assign the first byte of<br>response to either IPMI_OK or IPMI_CC_PARM_NOT_SUPPORTED and you're using<br>memcpy.<br><br>Wouldn't it be nicer to<br>memset(response, 0, *data_len);<br>(or not even memset since NUM_RETURN_BYTES_OF_SET is 1) and<br>*response = IPMI_OK;<br>or<br>*response = IPMI_CC_PARM_NOT_SUPPORTED;<br>and avoid having output_buf at all?<br><br>Which makes me wonder why response is actually a void *, is this a good idea?<br><br>> }<br>> <br>> return rc;<br>> }<br>> <br>> -void register_netfn_chassis_functions()<br>> +void register_netfn_chassis_functions ()<br>> {<br>> - printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> - ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);<br>> + printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> + ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard);<br>> <br>> - printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);<br>> - ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);<br>> + printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS);<br>> + ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_GET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_get_sys_boot_options);<br>> <br>> - printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);<br>> - ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);<br>> + printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS);<br>> + ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_SET_SYS_BOOT_OPTIONS, NULL, ipmi_chassis_set_sys_boot_options);<br>> +<br>> + printf ("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL);<br>> + ipmi_register_callback (NETFUN_CHASSIS, IPMI_CMD_CHASSIS_CONTROL, NULL, ipmi_chassis_control);<br>> }<br>> +<br>> diff --git a/chassishandler.h b/chassishandler.h<br>> index 1a26411..a310741 100644<br>> --- a/chassishandler.h<br>> +++ b/chassishandler.h<br>> @@ -3,21 +3,39 @@<br>> <br>> #include <stdint.h><br>> <br>> +// TODO: Petitboot requires 8 bytes of response<br>> +// however only 5 of them are used. The remaining<br>> +// 3 bytes are not used in petitboot and the value<br>> +// of them are all zero.<br>> +#define NUM_RETURN_BYTES_OF_GET 8<br>> +#define NUM_RETURN_BYTES_OF_GET_USED 5<br>> +#define NUM_RETURN_BYTES_OF_SET 1<br>> +#define NUM_INPUT_BYTES_OF_SET 5<br>> +<br>> // IPMI commands for Chassis net functions.<br>> enum ipmi_netfn_app_cmds<br>> {<br>> // Chassis Control<br>> IPMI_CMD_CHASSIS_CONTROL = 0x02,<br>> // Get capability bits<br>> + IPMI_CMD_SET_SYS_BOOT_OPTIONS = 0x08,<br>> IPMI_CMD_GET_SYS_BOOT_OPTIONS = 0x09,<br>> };<br>> <br>> // Command specific completion codes<br>> enum ipmi_chassis_return_codes<br>> {<br>> + IPMI_OK = 0x0,<br>> IPMI_CC_PARM_NOT_SUPPORTED = 0x80,<br>> };<br>> <br>> +// Generic completion codes,<br>> +// see IPMI doc section 5.2<br>> +enum ipmi_generic_return_codes<br>> +{<br>> + IPMI_OUT_OF_SPACE = 0xC4,<br>> +};<br>> +<br>> // Various Chassis operations under a single command.<br>> enum ipmi_chassis_control_cmds : uint8_t<br>> {</font><br> </div></blockquote>
<div dir="ltr" > </div></div><BR>