<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" >Hi Sam,</div>
<div dir="ltr" > </div>
<div dir="ltr" >Thanks for looking at the code.</div>
<div dir="ltr" > </div>
<div dir="ltr" >Please find the response inline.</div>
<div dir="ltr" > </div>
<div dir="ltr" ><font face="Default Monospace,Courier New,Courier,monospace" size="2" >- What is the purpose of the host_network_config struct, and why is the<br> request parsed into a string before being stored? Why not just store<br> the original request as is?</font></div>
<div dir="ltr" > </div>
<div dir="ltr" ><font face="Default Monospace,Courier New,Courier,monospace" size="2" > RG>>>This host config data is configured through REST/HOST(IPMI Client),Through REST we are configuring the data in human readable string,It is cumbersome for the user to enter the data in correct format in Hexadecimal through REST.</font></div>
<div dir="ltr" ><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >- Do you have tests to verify the correctness of the hex produced?</font></div>
<div dir="ltr" ><font face="Default Monospace,Courier New,Courier,monospace" size="2" > RG>>I veri</font>fied it in my unit testing that it was working fine.I was setting the data through REST and getting it via IPMI and viceversa also.</div>
<div dir="ltr" > </div>
<div dir="ltr" > Is there a path where you think that Hex will not be correct? Please let me know so that I can look into it.</div>
<div dir="ltr" > </div>
<div dir="ltr" > </div>
<div dir="ltr" > Regards</div>
<div dir="ltr" > Ratan Gupta</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: Samuel Mendoza-Jonas <sam@mendozajonas.com><br>To: OpenBMC Patches <openbmc-patches@stwcx.xyz><br>Cc: openbmc@lists.ozlabs.org, Ratan K Gupta/India/IBM@IBMIN<br>Subject: Re: [PATCH phosphor-host-ipmid v5] Resolves openbmc/openbmc#267'<br>Date: Fri, Jun 17, 2016 11:17 AM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >Hi Ratan,<br><br>I have some notes below, but mainly I have these two questions:<br><br>- What is the purpose of the host_network_config struct, and why is the<br> request parsed into a string before being stored? Why not just store<br> the original request as is?<br>- Do you have tests to verify the correctness of the hex produced?<br><br>Cheers,<br>Sam<br><br>On Thu, Jun 16, 2016 at 09:50:40AM -0500, OpenBMC Patches wrote:<br>> From: ratagupt <ratagupt@in.ibm.com><br>><br>> ---<br>> chassishandler.C | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++----<br>> 1 file changed, 200 insertions(+), 12 deletions(-)<br>><br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index d5b3404..2cc0895 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -1,17 +1,25 @@<br>> #include "chassishandler.h"<br>> #include "ipmid-api.h"<br>> #include <stdio.h><br>> +#include <stdlib.h><br>> #include <string.h><br>> #include <stdint.h><br>> -<br>> -<br>> +#include <arpa/inet.h><br>> +#include <netinet/in.h><br>> +#include <string><br>> +#include <sstream><br>> +#include <array><br>> +using namespace std;<br>> //Defines<br>> -#define SET_PARM_VERSION 1<br>> -#define SET_PARM_BOOT_FLAGS_PERMANENT 0x40 //boot flags data1 7th bit on<br>> +#define SET_PARM_VERSION 0x01<br>> +#define SET_PARM_BOOT_FLAGS_PERMANENT 0x40 //boot flags data1 7th bit on<br>> #define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME 0x80 //boot flags data1 8th bit on<br>> #define SET_PARM_BOOT_FLAGS_VALID_PERMANENT 0xC0 //boot flags data1 7 & 8 bit on<br>> <br>> -<br>> +#define SIZE_MAC 18<br>> +#define SIZE_HOST_NETWORK_DATA 26<br><br>How did you decide on the size for SIZE_HOST_NETWORK_DATA?<br><br>> +#define SIZE_BOOT_OPTION SIZE_HOST_NETWORK_DATA<br>> +#define SIZE_PREFIX 7<br>> <br>> // OpenBMC Chassis Manager dbus framework<br>> const char *chassis_bus_name = "org.openbmc.control.Chassis";<br>> @@ -233,14 +241,174 @@ struct get_sys_boot_options_t {<br>> struct get_sys_boot_options_response_t {<br>> uint8_t version;<br>> uint8_t parm;<br>> - uint8_t data[5];<br>> + uint8_t data[SIZE_BOOT_OPTION];<br>> } __attribute__ ((packed));<br>> <br>> struct set_sys_boot_options_t {<br>> uint8_t parameter;<br>> - uint8_t data[8];<br>> + uint8_t data[SIZE_BOOT_OPTION];<br>> } __attribute__ ((packed));<br>> <br>> +struct host_network_config_t {<br>> + string ipaddress;<br>> + string prefix;<br>> + string gateway;<br>> + string macaddress;<br>> + string isDHCP;<br><br>Is there a need to have these all as strings?<br><br>> +<br>> + host_network_config_t():ipaddress(),prefix(),gateway(),macaddress() {}<br>> +};<br>> +<br>> +uint8_t getHostNetworkData(get_sys_boot_options_response_t *respptr)<br>> +{<br>> + char *prop = NULL;<br>> +<br>> + std::array<uint8_t, SIZE_BOOT_OPTION> respData{0x80,0x21, 0x70 ,0x62 ,0x21,0x00 ,0x01 ,0x06 ,0x04};<br><br>The "0x21, 0x70 ,0x62 ,0x21" cookie is a magic value that Petitboot uses<br>to recognise an override message it can read. Using it unconditionally<br>here means that openbmc would be hard-coded to only support Petitboot,<br>is that what we, or other users would expect?<br><br>> +<br>> + int rc = dbus_get_property("network_config",&prop);<br>> +<br>> + if (rc < 0) {<br>> + fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");<br>> + return rc;<br>> + }<br>> +<br>> + /* network_config property Value would be in the form of<br>> + * ipaddress=1.1.1.1,prefix=16,gateway=2.2.2.2,mac=11:22:33:44:55:66,dhcp=0<br>> + */<br>> +<br>> + /* Parsing the string and fill the hostconfig structure with the<br>> + * values */<br><br>If I'm reading this right the host_config structure is filled in<br>according to the "network_config" string, and then each field is copied<br>into respData - would it be simpler to skip the structure and just<br>fill in respData?<br><br>> +<br>> + host_network_config_t host_config;<br>> + string delimiter = ",";<br>> +<br>> + size_t pos = 0;<br>> + string token,name,value;<br>> + string conf_str(prop);<br>> + <br>> + printf ("Configuration String[%s]\n ",conf_str.c_str());<br><br>Should we print this all the time, or just on debug?<br><br>> +<br>> + while ( conf_str.length() > 0) {<br>> +<br>> + pos = conf_str.find(delimiter);<br>> +<br>> + //This condition is to extract the last<br>> + //Substring as we will not be having the delimeter<br>> + //at end. std::string::npos is -1<br>> + <br>> + if ( pos == std::string::npos )<br>> + {<br>> + pos = conf_str.length();<br>> + }<br>> + token = conf_str.substr(0, pos);<br>> + int pos1 = token.find("=");<br>> +<br>> + name = token.substr(0,pos1);<br>> + value = token.substr(pos1+1,pos);<br>> +<br>> + if ( name == "ipaddress" )<br>> + host_config.ipaddress = value;<br>> + else if ( name == "prefix")<br>> + host_config.prefix = value;<br>> + else if ( name == "gateway" )<br>> + host_config.gateway = value;<br>> + else if ( name == "mac" )<br>> + host_config.macaddress = value;<br>> + else if ( name == "dhcp" )<br>> + host_config.isDHCP = value;<br>> + <br>> + conf_str.erase(0, pos + delimiter.length());<br>> + }<br>> +<br>> + //Starting from index 9 as 9 bytes are prefilled.<br>> +<br>> + pos = 0;<br>> + delimiter = ":";<br>> + uint8_t resp_byte = 0;<br>> + uint8_t index = 9;<br>> + <br>> + while ((pos = host_config.macaddress.find(delimiter)) != std::string::npos) {<br>> +<br>> + token = host_config.macaddress.substr(0, pos);<br>> + resp_byte = strtoul(token.c_str(), NULL, 16);<br><br>Would be a good idea to check strtoul for errors<br><br>> + memcpy((void*)&respData[index], &resp_byte, 1);<br><br>Don't need to cast to (void*)<br><br>> + host_config.macaddress.erase(0, pos + delimiter.length());<br>> + index++;<br>> + }<br>> +<br>> + resp_byte = strtoul(host_config.macaddress.c_str(), NULL, 16);<br>> + memcpy((void*)&respData[index++], &resp_byte, 1);<br><br>Same<br><br>> +<br>> + //Conevrt the dhcp,ipaddress,mask and gateway as hex number<br><br>Typo, Conevrt/Convert<br><br>> + respData[index++]=0x00;<br>> + sscanf(host_config.isDHCP.c_str(),"%02X",&respData[index++]);<br>> +<br>> + inet_pton(AF_INET,host_config.ipaddress.c_str(),(void *)&respData[index]);<br>> + index+=4;<br>> +<br>> + sscanf(host_config.prefix.c_str(),"%02d",&respData[index++]);<br>> + inet_pton(AF_INET,host_config.gateway.c_str(),(void *)&respData[index]);<br>> + index+=4;<br>> + <br>> + printf ("\n===Printing the IPMI Formatted Data========\n");<br><br>As above, debug message?<br><br>> +<br>> + for (int j = 0;j<index;j++)<br><br>Spacing it out like this is easier to read:<br><br> for (int j = 0; j < index; j++)<br><br>> + printf("%02x ", respData[j]);<br>> +<br>> + memcpy(respptr->data,respData.data(),SIZE_BOOT_OPTION);<br>> + return 0;<br>> +}<br>> +<br>> +uint8_t setHostNetworkData(set_sys_boot_options_t * reqptr)<br>> +{<br>> + string host_network_config;<br>> + char mac[SIZE_MAC] = {0};<br>> + char ipAddress[INET_ADDRSTRLEN] = {0};<br>> + char gateway[INET_ADDRSTRLEN] = {0};<br>> + char dhcp[SIZE_PREFIX] = {0};<br>> + char prefix[SIZE_PREFIX] = {0};<br>> + uint32_t cookie = 0;<br>> +<br>> + memcpy(&cookie,(char *)&(reqptr->data[1]),sizeof(cookie));<br>> + <br>> + uint8_t index = 9; <br>> +<br>> + if ( cookie) {<br>> + <br>> + snprintf(mac, SIZE_MAC, "%02x:%02x:%02x:%02x:%02x:%02x",<br>> + reqptr->data[index],<br>> + reqptr->data[index+1],<br>> + reqptr->data[index+2],<br>> + reqptr->data[index+3],<br>> + reqptr->data[index+4],<br>> + reqptr->data[index+5]);<br>> + <br>> + snprintf(dhcp,SIZE_PREFIX, "%d", reqptr->data[index+7]);<br><br>*If* this is following the format used by Petitboot, is the order of<br>fields here correct? It looks like the size fields have been skipped.<br><br>> +<br>> + snprintf(ipAddress, INET_ADDRSTRLEN, "%d.%d.%d.%d",<br>> + reqptr->data[index+8], reqptr->data[index+9], reqptr->data[index+10], reqptr->data[index+11]);<br>> +<br>> + snprintf(prefix,SIZE_PREFIX,"%d", reqptr->data[index+12]);<br><br>What is the prefix? Is it meant to be the subnet mask?<br><br>> +<br>> + snprintf(gateway, INET_ADDRSTRLEN, "%d.%d.%d.%d",<br>> + reqptr->data[index+13], reqptr->data[index+14], reqptr->data[index+15], reqptr->data[index+16]);<br>> + }<br>> +<br>> + host_network_config += "ipaddress="+string(ipAddress)+",prefix="+ \<br>> + string(prefix)+",gateway="+string(gateway)+\<br>> + ",mac="+string(mac)+",dhcp="+string(dhcp);<br>> +<br>> + printf ("Network configuration changed: %s\n",host_network_config.c_str());<br>> +<br>> + int r = dbus_set_property("network_config",host_network_config.c_str());<br>> +<br>> + if (r < 0) {<br>> + fprintf(stderr, "Dbus set property(network_config) failed for set_sys_boot_options.\n");<br>> + r = IPMI_CC_UNSPECIFIED_ERROR;<br>> + }<br>> + return r;<br>> +}<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>> @@ -446,7 +614,18 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> }<br>> <br>> <br>> - } else {<br>> + } else if ( reqptr->parameter == 0x61 ) {<br>> + resp->parm = 0x61;<br>> + int ret = getHostNetworkData(resp);<br>> + if (ret < 0) {<br>> + fprintf(stderr, "getHostNetworkData failed for get_sys_boot_options.\n");<br>> + rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> +<br>> + }else<br>> + rc = IPMI_CC_OK;<br>> + }<br>> +<br>> + else {<br>> fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> }<br>> <br>> @@ -464,11 +643,10 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> {<br>> ipmi_ret_t rc = IPMI_CC_OK;<br>> char *s;<br>> -<br>> - printf("IPMI SET_SYS_BOOT_OPTIONS\n");<br>> -<br>> set_sys_boot_options_t *reqptr = (set_sys_boot_options_t *) request;<br>> <br>> + printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);<br>> +<br>> // This IPMI command does not have any resposne data<br>> *data_len = 0;<br>> <br>> @@ -476,6 +654,7 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<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>> +<br><br>Extra line<br><br>> if (reqptr->parameter == 5) {<br>> <br>> s = get_boot_option_by_ipmi(((reqptr->data[1] & 0x3C) >> 2));<br>> @@ -507,7 +686,16 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> }<br>> <br>> - } else {<br>> + }<br>> + else if ( reqptr->parameter == 0x61 ) {<br>> + printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);<br>> + int ret = setHostNetworkData(reqptr);<br>> + if (ret < 0) {<br>> + fprintf(stderr, "setHostNetworkData failed for set_sys_boot_options.\n");<br>> + rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> + }<br>> + } <br>> + else {<br>> fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> rc = IPMI_CC_PARM_NOT_SUPPORTED;<br>> }<br>> --<br>> 2.8.4<br>><br>><br>> _______________________________________________<br>> openbmc mailing list<br>> openbmc@lists.ozlabs.org<br>> <a href="https://lists.ozlabs.org/listinfo/openbmc" target="_blank" >https://lists.ozlabs.org/listinfo/openbmc</a></font></div></blockquote>
<div dir="ltr" > </div></div></div><BR>