<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" >For the whitespace introduced and the coding style issue, I will try to propose a fix to it, hopefully with minimum coding style changes.</div>
<div dir="ltr" > </div>
<div dir="ltr" >For other issues, we can continue discuss it but I believe we can firstly accept this patch and then revisit them if there is really concerns for them.</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: Peng Fei BG Gou/China/IBM@IBMCN<br>Cc: openbmc@lists.ozlabs.org, openbmc-patches@stwcx.xyz<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 3:12 PM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Wed, 6 Jan 2016 06:13:06 +0000<br>"Peng Fei BG Gou" <shgoupf@cn.ibm.com> wrote:<br><br>> Hey Cyril,<br>> <br>> Thanks for your detailed review on this patch.<br>> <br><br>Hi Peng,<br><br>> Just want to answer (most of) your questions in batch:<br>> 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.<br><br>The set/get boot option is fine as one patch (or two... not too fussed really) I<br>agree.<br><br>You do still have whitespace (and other) changes which touch code not related<br>to the boot option work, notably in ipmi_chassis_power_control() and<br>ipmi_chassis_control()<br><br>> 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.<br><br>I'm not saying sscanf won't work, I completely agree that it will. I do<br>disagree that there isn't a performance concern, if I'm not mistaken this code<br>will be running on an ASPEED2400 which is a single core ARM chip running at<br>400MHz. We have breathing space and we don't need to write absolutely<br>everything to save all the possible cycles but lets not waste the little<br>breathing space we do have.<br><br>> 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.<br><br>Again, I'm not saying it won't work but I'm trying to understand why the<br>complexity in these signatures, it doesn't seem like it's being used and<br>pointless complexity doesn't help anyone.<br><br>However, if there is a good reason for these complicated signatures I'm all for<br>it, if you could document them in<br><a href="https://github.com/openbmc/docs/blob/master/dbus-interfaces.md" target="_blank" >https://github.com/openbmc/docs/blob/master/dbus-interfaces.md</a> that would be<br>fantastic.<br><br>Thanks.<br><br>> 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.<br>> 5) For the way how return code is handled, I believe it is working fine.<br>> 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.<br>> <br><br>It isn't a major issue at all but this project already has enough code style<br>issues as it is, I don't see why patches should be merged that introduce more<br>style problems.<br><br>Perhaps you should run your auto style tool on your patch(es) before submission?<br><br>> 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.<br>> <br>> GOU, Peng Fei (苟鹏飞), Ph.D.<br>> OpenPower Team.<br>> +86-21-609-28631<br>> <br>> <br>> ----- 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>> <br>> 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>> > {<br>> <br>> <br>><br>> </font><br> </div></blockquote>
<div dir="ltr" > </div></div><BR>