<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Thanks Stewart for your comments.</div>
<div dir="ltr" > </div>
<div dir="ltr" >Please see my answers starting with >>>> [pengfei]: </div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<div dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div 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" data-history-expanded="1" dir="ltr" style="border-left:solid #aaaaaa 2px; margin-left:5px; padding-left:5px; direction:ltr; margin-right:0px" >----- Original message -----<br>From: Stewart Smith <stewart@linux.vnet.ibm.com><br>To: OpenBMC Patches <openbmc-patches@stwcx.xyz>, openbmc@lists.ozlabs.org<br>Cc: Peng Fei BG Gou/China/IBM@IBMCN<br>Subject: Re: [PATCH phosphor-host-ipmid v5] Add get/set boot option ipmid command support with correct DBUS property handling.<br>Date: Thu, Jan 7, 2016 1:52 PM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >OpenBMC Patches <openbmc-patches@stwcx.xyz> writes:<br>> From: shgoupf <shgoupf@cn.ibm.com><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>> 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>A commit message should describe the change/feature being implemented<br>and provide additional explanation, this reads like the story of how you<br>got to this point rather than describing what's going on.</font><br> </div>
<div>>>>> [pengfei]: Good point. I will cleanup the comments in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> diff --git a/chassishandler.C b/chassishandler.C<br>> index 1389db9..8d516de 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -5,9 +5,233 @@<br><snip><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>> + 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>> + uint64_t return_value;<br>> + sscanf(buf, "%llX", &return_value);<br><br>should this be PRIx64 ?</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Good point. I will fix in my next update.<br><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> +<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>> + } 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>> +<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",<br>> *data_len);<br><br>these look like debugging printouts? Is there any plan to move to<br>something better than printing to stdout?</font></div>
<div> </div>
<div>>>>> [pengfei]: I cannot figure out a better way to do this given the current framework. So I will delete these printf in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> +<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>snprintf is better as it can be more obviously correct, or rather, more<br>obvious to see that it's not an outright buffer overflow.<br><br>You also want to use C99 PRIx64 as otherwise this is incorrect on some<br>platforms and will emit a warning.</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Good point. I will fix in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> +<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>> + return IPMI_CC_PARM_NOT_SUPPORTED;<br><br>Keep whitespace fixes in a separate patch.</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Good point. I will fix in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> }<br>> <br>> return rc;<br>> @@ -135,12 +449,16 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> <br>> void register_netfn_chassis_functions()<br>> {<br>> - printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> + printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n", NETFUN_CHASSIS,<br>> IPMI_CMD_WILDCARD);<br><br>same here, separate patch for pure whitespace fix.</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Good point. I will fix in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> 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>> + 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>> + 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,<br>> NULL, ipmi_chassis_control);<br><br>and teh same for these.</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Good point. I will fix in my next update.</div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> }<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><br>where? why? why will it never change?</font></div>
<div> </div>
<div><div>>>>> [pengfei]: Currently the target of this patch is to add support for what the current petitboot requires, so I don't believe we need to do things beyond that with this patch.</div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >--<br>Stewart Smith<br>OPAL Architect, IBM.</font></div></div></div></div></div></div></blockquote>
<div dir="ltr" > </div></div></div><BR>