<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" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" ><div>Hi Cyrl,</div>
<div> </div>
<div>Thanks for reviewing the code.</div>
<div> </div>
<div>I spoke to the maintainer of the host-ipmid module and was told that we can use C++/C interchangeably based on the use cases.</div>
<div> </div>
<div>I also noticed that we are using the g++ compiler in the makefile of host-ipmid module so thought we an use c++.</div>
<div>Please find my comments inline.</div>
<div> </div>
<div>Regards</div>
<div>Ratan</div></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: Cyril Bur <cyrilbur@gmail.com><br>To: Ratan K Gupta/India/IBM@IBMIN<br>Cc: openbmc@lists.ozlabs.org<br>Subject: Re: [PATCH phosphor-host-ipmid] Implement Network Override<br>Date: Tue, Jun 7, 2016 7:00 AM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Mon, 6 Jun 2016 13:00:44 -0500<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>> From: ratagupt <ratagupt@in.ibm.com><br>><br><br>Please see: <a href="https://lists.ozlabs.org/pipermail/openbmc/2016-May/003240.html" target="_blank" >https://lists.ozlabs.org/pipermail/openbmc/2016-May/003240.html</a> I'm<br>not going to bother explaining (some) things, if I'm not clear it's because you<br>didn't read that email and associated links<br><br>> ---<br>> chassishandler.C | 228 +++++++++++++++++++++++++++++++++++++++++++++++++++++--<br>> 1 file changed, 220 insertions(+), 8 deletions(-)<br>><br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index d5b3404..e21d6da 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -3,15 +3,19 @@<br>> #include <stdio.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>> +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_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>> +#define SIZE_BOOT_OPTION SIZE_HOST_NETWORK_DATA<br>> +//#define INET_ADDRSTRLEN 15<br>> <br>> // OpenBMC Chassis Manager dbus framework<br>> const char *chassis_bus_name = "org.openbmc.control.Chassis";<br>> @@ -233,14 +237,197 @@ 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>This is a packed struct, so, this means it could represent a structure in<br>memory or a file, I can't see how big SIZE_BOOT_OPTION is but is it 5? Later<br>you use it for a data field that had size 8, so if you ever want to memcpy for<br>sizeof(struct get_sys_boot_options_response_t) or sizeof(struct<br>set_sys_boot_options_t) will this work?<br><br>Picture this scenario; process A (which you have no control over) writes a<br>struct get_sys_boot_options_response_t to a file and it had data[5], process<br>B (you) now try to read that file and cast your buffer to a struct<br>get_sys_boot_options_response_t but you used data[SIZE_BOOT_OPTION] and you<br>walk through 'for (i = 0; i < SIZE_BOOT_OPTION; i++)' completely correct from<br>your point of view, your structure should be big enough, but what was written<br>to file wasn't big enough for you to do that... what happens? Now imagine the<br>file was mmaped... what happens?</font><br> </div>
<div>RG>>Here Before my fix the SetSystem boot option response data was 8 byte long and the getSystemBootOption Data was 5 Byte long.It is all about the IPMI protocol how much byte needs to be send or is it to be received. </div></blockquote>
<div> RG>>Now with the fix I have modified both(getSystemBootOption/SetSystem boot option Data) to 26 bytes long.Now if the other process sends me the 20byte/5byte.Our code is taking care.if the other process sends more then 26 byte,we are ignoring that data as it is not as per the protocol.</div>
<div> </div>
<div> </div>
<div> </div>
<blockquote class="history-quote-1465287871325" 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" ><div> </div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> <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>> + host_network_config_t()<br>> + {<br>> + ipaddress = "";<br>> + prefix = "";<br>> + gateway = "";<br>> + macaddress = "";<br>> + isDHCP = "";<br>> + }<br><br>Admittedly I don't do c++, is this c++? Do c++ programmers recommend this?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" ><strong>RG>>>>>In C++ it is valid contruct.</strong><br><br>> +};<br>> +<br>> +uint8_t getHostNetworkData(get_sys_boot_options_response_t* respptr)<br><br>Style<br><br>> +{<br>> +<br>> +<br>> + printf ("Inside getHostNetworkData");<br><br>Style, also that looks a lot like a debug printf...?<br><br>> + char * prop = NULL;<br><br>Style<br><br>> +<br>> + uint8_t data[SIZE_BOOT_OPTION]={0x80,0x21, 0x70 ,0x62 ,0x21,0x00 ,0x01 ,0x06 ,0x04};<br><br>uint8_t data[SIZE_BOOT_OPTION] = {0x80, 0x21, 0x70, 0x62, 0x21, 0x00, 0x01,<br>0x06, 0x04};<br><br>> +<br>> + //As first 9 bytes are prefilled so in rest of the bytes we are assigning 0<br>> +<br>> + memset( data+9 ,0,(SIZE_BOOT_OPTION-9) );<br><br>"All array elements that are not initialized explicitly are initialized<br>implicitly the same way as objects that have static storage duration."<br><br><a href="http://en.cppreference.com/w/c/language/array_initialization" target="_blank" >http://en.cppreference.com/w/c/language/array_initialization</a> <br><br>so what you've done is pointless...</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" ><strong>RG>>Accepted</strong><br><br>> +<br>> + int rc = dbus_get_property("network_config",&prop);<br><br>int rc = dbus_get_property("network_config", &prop);<br><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>> + host_network_config_t host_config;<br>> + string delimiter = ",";<br>> +<br>> + int 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>> + while ((pos = conf_str.find(delimiter)) != std::string::npos) {<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><br>There needs to be some kind of consistency as to what language we're writing in<br>because that line can't persist. I understand that in c++ this will do what you<br>want, however, to a C programmer screams alarm bells, this compares pointers<br>which is almost never what you want to do.<br><br>In fairness this problem started long before this patch but that's no reason to<br>continue it. I'm flexible, I like to think I'm actually not that strongly<br>opinionated (at least on this issue), please pick one!<br><br>Looking at the language breakdown that github seems to think this project is<br>written in:<br>C 97.2% Makefile 1.6% Other 1.2%<br><br>How about we go exclusively with C?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" ><strong>RG>>>This needs to be discussed with the stakeholders.</strong><br><br>Maintainers: Can we try to enforce this at a Makefile level?<br><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>> + printf ("Name=[%s],Value=[%s],position=[%d]\n",name.c_str(),value.c_str(),pos);<br>> + }<br>> +<br>> + //Converting the mac address as number//<br>> + //If there as an error in converting the mac as number we are not throwing<br>> + //error we would be sending 0's for the mac.<br>> + <br>> + char *tokptr = NULL;<br>> + char* digit = strtok_r((char *)host_config.macaddress.c_str(), ":", &tokptr);<br>> + if (digit == NULL)<br>> + {<br>> + fprintf(stderr, "Unexpected MAC format: %s", host_config.macaddress.c_str());<br>> + }<br>> + <br>> + //As 9 bytes are pre filled so staring from index 9.If there is a failure<br>> + //in the strtok_r then digit will be null then we don't need to do<br>> + //anything as data is prefilled with 0<br>> + <br>> + uint8_t index=9;<br>> +<br>> + while (digit != NULL)<br>> + {<br>> + int resp_byte = strtoul(digit, NULL, 16);<br>> + memcpy((void*)&data[index], &resp_byte, 1);<br>> + index++;<br>> + digit = strtok_r(NULL, ":", &tokptr);<br>> + }<br>> +<br>> + //Conevrt the dhcp,ipaddress,mask and gateway as hex number<br>> + data[index++]=0x00;<br>> + sscanf(host_config.isDHCP.c_str(),"%02X",&data[index++]);<br>> +<br>> + inet_pton(AF_INET,host_config.ipaddress.c_str(),(void *)&data[index]);<br>> + index+=4;<br>> + sscanf(host_config.prefix.c_str(),"%02X",&data[index++]);<br>> + inet_pton(AF_INET,host_config.gateway.c_str(),(void *)&data[index]);<br>> + index+=4;<br>> +<br>> + printf ("\n===========Printing the Network Conf=====================\n");<br>> +<br>> + for (int j = 0;j<index;j++)<br>> + printf("%02x ", data[j]);<br>> +<br>> + memcpy(respptr->data,data,SIZE_BOOT_OPTION);<br>> + return 0;<br>> +<br>> +}<br>> +<br>> +uint8_t setHostNetworkData(set_sys_boot_options_t * reqptr)<br>> +{<br>> + printf ("\n Inside setHostNetworkData***********");<br>> + string host_network_config;<br>> + char mac[SIZE_MAC];<br>> + char ipAddress[INET_ADDRSTRLEN];<br>> + char gateway[INET_ADDRSTRLEN];<br>> + char prefix[1];<br>> + char dhcp[2];<br>> + uint32_t cookie = 0;<br>> +<br>> +<br>> + memset( mac ,0,SIZE_MAC );<br>> + memset(ipAddress,0,INET_ADDRSTRLEN);<br>> + memset(gateway,0,INET_ADDRSTRLEN);<br>> + memset(prefix,0,1);<br>> + memset(dhcp,0,2);<br>> +<br>> + uint8_t index = 9; <br>> + sscanf((char *)&(reqptr->data[1]),"%02X",&cookie);<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>> +<br>> + snprintf(dhcp,2, "%d", reqptr->data[index+6]);<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, INET_ADDRSTRLEN, "%d", reqptr->data[index+12]);<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>> +<br>> + printf ("Host Config Str= %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>> +<br>> + return r;<br>> +<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 +633,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>> @@ -476,6 +674,9 @@ 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>> + printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);<br>> if (reqptr->parameter == 5) {<br>> <br>> s = get_boot_option_by_ipmi(((reqptr->data[1] & 0x3C) >> 2));<br>> @@ -507,7 +708,18 @@ 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>> + } <br>> + <br>> + else {<br>> fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> rc = IPMI_CC_PARM_NOT_SUPPORTED;<br>> }</font></div></blockquote>
<div dir="ltr" > </div></div></div></div><BR>