[PATCH phosphor-host-ipmid v5] Implement Network Override

[PATCH phosphor-host-ipmid v5] Implement Network Override

[OpenBMC Patches]

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/openbmc/phosphor-host-ipmid/90)
<!-- Reviewable:end -->


https://github.com/openbmc/phosphor-host-ipmid/pull/90

ratagupt (1):
  Resolves openbmc/openbmc#267'

 chassishandler.C | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 200 insertions(+), 12 deletions(-)

-- 
2.8.4

[PATCH phosphor-host-ipmid v5] Resolves openbmc/openbmc#267'

[OpenBMC Patches]

From: ratagupt <ratagupt@in.ibm.com>

---
 chassishandler.C | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 200 insertions(+), 12 deletions(-)

diff --git a/chassishandler.C b/chassishandler.C
index d5b3404..2cc0895 100644
--- a/chassishandler.C
+++ b/chassishandler.C
@@ -1,17 +1,25 @@
 #include "chassishandler.h"
 #include "ipmid-api.h"
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <stdint.h>
-
-
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <string>
+#include <sstream>
+#include <array>
+using namespace std;
 //Defines
-#define SET_PARM_VERSION 1
-#define SET_PARM_BOOT_FLAGS_PERMANENT 0x40 //boot flags data1 7th bit on
+#define SET_PARM_VERSION                     0x01
+#define SET_PARM_BOOT_FLAGS_PERMANENT        0x40 //boot flags data1 7th bit on
 #define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME   0x80 //boot flags data1 8th bit on
 #define SET_PARM_BOOT_FLAGS_VALID_PERMANENT  0xC0 //boot flags data1 7 & 8 bit on 
 
-
+#define SIZE_MAC                             18
+#define SIZE_HOST_NETWORK_DATA               26
+#define SIZE_BOOT_OPTION                     SIZE_HOST_NETWORK_DATA
+#define SIZE_PREFIX                          7
 
 // OpenBMC Chassis Manager dbus framework
 const char  *chassis_bus_name      =  "org.openbmc.control.Chassis";
@@ -233,14 +241,174 @@ struct get_sys_boot_options_t {
 struct get_sys_boot_options_response_t {
     uint8_t version;
     uint8_t parm;
-    uint8_t data[5];
+    uint8_t data[SIZE_BOOT_OPTION];
 }  __attribute__ ((packed));
 
 struct set_sys_boot_options_t {
     uint8_t parameter;
-    uint8_t data[8];
+    uint8_t data[SIZE_BOOT_OPTION];
 }  __attribute__ ((packed));
 
+struct host_network_config_t {
+     string ipaddress;
+     string prefix;
+     string gateway;
+     string macaddress;
+     string isDHCP;
+
+     host_network_config_t():ipaddress(),prefix(),gateway(),macaddress() {}
+};
+
+uint8_t getHostNetworkData(get_sys_boot_options_response_t *respptr)
+{
+    char *prop = NULL;
+
+    std::array<uint8_t, SIZE_BOOT_OPTION> respData{0x80,0x21, 0x70 ,0x62 ,0x21,0x00 ,0x01 ,0x06 ,0x04};
+
+    int rc = dbus_get_property("network_config",&prop);
+
+    if (rc < 0) {
+        fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");
+        return rc;
+    }
+
+    /* network_config property Value would be in the form of 
+     * ipaddress=1.1.1.1,prefix=16,gateway=2.2.2.2,mac=11:22:33:44:55:66,dhcp=0
+     */
+
+    /* Parsing the string and fill the hostconfig structure with the 
+     * values */
+
+    host_network_config_t host_config;
+    string delimiter = ",";
+
+    size_t pos = 0;
+    string token,name,value;
+    string conf_str(prop);
+    
+    printf ("Configuration String[%s]\n ",conf_str.c_str());
+
+    while ( conf_str.length() > 0) {
+
+        pos = conf_str.find(delimiter);
+
+        //This condition is to extract the last 
+        //Substring as we will not be having the delimeter 
+        //at end. std::string::npos is -1
+        
+        if ( pos == std::string::npos )
+        { 
+           pos = conf_str.length();
+        }
+        token = conf_str.substr(0, pos);
+        int pos1 = token.find("=");
+
+        name = token.substr(0,pos1);
+        value = token.substr(pos1+1,pos);
+
+        if ( name == "ipaddress" )
+            host_config.ipaddress = value;
+        else if ( name == "prefix")
+            host_config.prefix = value; 
+        else if ( name == "gateway" )
+            host_config.gateway = value;
+        else if ( name == "mac" )
+            host_config.macaddress = value;
+        else if ( name == "dhcp" )
+            host_config.isDHCP = value;
+        
+        conf_str.erase(0, pos + delimiter.length());
+    }
+
+    //Starting from index 9 as 9 bytes are prefilled.
+
+    pos = 0; 
+    delimiter = ":";
+    uint8_t resp_byte = 0;
+    uint8_t index = 9;
+    
+    while ((pos = host_config.macaddress.find(delimiter)) != std::string::npos) {
+
+        token = host_config.macaddress.substr(0, pos);
+        resp_byte = strtoul(token.c_str(), NULL, 16);
+        memcpy((void*)&respData[index], &resp_byte, 1);
+        host_config.macaddress.erase(0, pos + delimiter.length());
+        index++;
+    }
+
+    resp_byte = strtoul(host_config.macaddress.c_str(), NULL, 16);
+    memcpy((void*)&respData[index++], &resp_byte, 1);
+
+    //Conevrt the dhcp,ipaddress,mask and gateway as hex number
+    respData[index++]=0x00;
+    sscanf(host_config.isDHCP.c_str(),"%02X",&respData[index++]);
+
+    inet_pton(AF_INET,host_config.ipaddress.c_str(),(void *)&respData[index]);
+    index+=4;
+
+    sscanf(host_config.prefix.c_str(),"%02d",&respData[index++]);
+    inet_pton(AF_INET,host_config.gateway.c_str(),(void *)&respData[index]);
+    index+=4; 
+    
+    printf ("\n===Printing the IPMI Formatted Data========\n");
+
+    for (int j = 0;j<index;j++)
+        printf("%02x ", respData[j]);
+
+    memcpy(respptr->data,respData.data(),SIZE_BOOT_OPTION);
+    return 0;
+}
+
+uint8_t setHostNetworkData(set_sys_boot_options_t * reqptr)
+{
+    string host_network_config;
+    char mac[SIZE_MAC] = {0};
+    char ipAddress[INET_ADDRSTRLEN] = {0};
+    char gateway[INET_ADDRSTRLEN] = {0};
+    char dhcp[SIZE_PREFIX] = {0};
+    char prefix[SIZE_PREFIX] = {0};
+    uint32_t cookie = 0;
+
+    memcpy(&cookie,(char *)&(reqptr->data[1]),sizeof(cookie));
+    
+    uint8_t index = 9;  
+
+    if ( cookie) {
+     
+            snprintf(mac, SIZE_MAC, "%02x:%02x:%02x:%02x:%02x:%02x",
+            reqptr->data[index],
+            reqptr->data[index+1],
+            reqptr->data[index+2],
+            reqptr->data[index+3],
+            reqptr->data[index+4],
+            reqptr->data[index+5]);
+    
+        snprintf(dhcp,SIZE_PREFIX, "%d", reqptr->data[index+7]);
+
+        snprintf(ipAddress, INET_ADDRSTRLEN, "%d.%d.%d.%d",
+            reqptr->data[index+8], reqptr->data[index+9], reqptr->data[index+10], reqptr->data[index+11]);
+
+        snprintf(prefix,SIZE_PREFIX,"%d", reqptr->data[index+12]);
+
+        snprintf(gateway, INET_ADDRSTRLEN, "%d.%d.%d.%d",
+            reqptr->data[index+13], reqptr->data[index+14], reqptr->data[index+15], reqptr->data[index+16]);
+    }
+
+    host_network_config += "ipaddress="+string(ipAddress)+",prefix="+ \
+                       string(prefix)+",gateway="+string(gateway)+\
+                       ",mac="+string(mac)+",dhcp="+string(dhcp);
+
+    printf ("Network configuration changed: %s\n",host_network_config.c_str());
+
+    int r = dbus_set_property("network_config",host_network_config.c_str());
+
+    if (r < 0) {
+        fprintf(stderr, "Dbus set property(network_config) failed for set_sys_boot_options.\n");
+        r = IPMI_CC_UNSPECIFIED_ERROR;
+    }
+    return r;
+}
+
 ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 
                               ipmi_request_t request, ipmi_response_t response, 
                               ipmi_data_len_t data_len, ipmi_context_t context)
@@ -446,7 +614,18 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
         }
 
 
-    } else {
+    } else if ( reqptr->parameter == 0x61 ) {
+       resp->parm      = 0x61;
+       int ret = getHostNetworkData(resp);
+       if (ret < 0) {
+           fprintf(stderr, "getHostNetworkData failed for get_sys_boot_options.\n");
+           rc = IPMI_CC_UNSPECIFIED_ERROR;
+
+       }else
+          rc = IPMI_CC_OK;
+    }
+
+    else {
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
     }
 
@@ -464,11 +643,10 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 {
     ipmi_ret_t rc = IPMI_CC_OK;
     char *s;
-
-    printf("IPMI SET_SYS_BOOT_OPTIONS\n");
-
     set_sys_boot_options_t *reqptr = (set_sys_boot_options_t *) request;
 
+    printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);
+
     // This IPMI command does not have any resposne data
     *data_len = 0;
 
@@ -476,6 +654,7 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
      * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.
      * This is the only parameter used by petitboot.
      */
+
     if (reqptr->parameter == 5) {
 
         s = get_boot_option_by_ipmi(((reqptr->data[1] & 0x3C) >> 2));
@@ -507,7 +686,16 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
             rc = IPMI_CC_UNSPECIFIED_ERROR;
         }
 
-    } else {
+    }
+    else if ( reqptr->parameter == 0x61 ) {
+       printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);
+       int ret = setHostNetworkData(reqptr);
+       if (ret < 0) {
+           fprintf(stderr, "setHostNetworkData failed for set_sys_boot_options.\n");
+           rc = IPMI_CC_UNSPECIFIED_ERROR;
+       }
+    }      
+    else {
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
         rc = IPMI_CC_PARM_NOT_SUPPORTED;
     }
-- 
2.8.4

Re: [PATCH phosphor-host-ipmid v5] Resolves openbmc/openbmc#267'

[Samuel Mendoza-Jonas]

Hi Ratan,

I have some notes below, but mainly I have these two questions:

- What is the purpose of the host_network_config struct, and why is the
  request parsed into a string before being stored? Why not just store
  the original request as is?
- Do you have tests to verify the correctness of the hex produced?

Cheers,
Sam

On Thu, Jun 16, 2016 at 09:50:40AM -0500, OpenBMC Patches wrote:
> From: ratagupt <ratagupt@in.ibm.com>
> 
> ---
>  chassishandler.C | 212 +++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 200 insertions(+), 12 deletions(-)
> 
> diff --git a/chassishandler.C b/chassishandler.C
> index d5b3404..2cc0895 100644
> --- a/chassishandler.C
> +++ b/chassishandler.C
> @@ -1,17 +1,25 @@
>  #include "chassishandler.h"
>  #include "ipmid-api.h"
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <string.h>
>  #include <stdint.h>
> -
> -
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +#include <string>
> +#include <sstream>
> +#include <array>
> +using namespace std;
>  //Defines
> -#define SET_PARM_VERSION 1
> -#define SET_PARM_BOOT_FLAGS_PERMANENT 0x40 //boot flags data1 7th bit on
> +#define SET_PARM_VERSION                     0x01
> +#define SET_PARM_BOOT_FLAGS_PERMANENT        0x40 //boot flags data1 7th bit on
>  #define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME   0x80 //boot flags data1 8th bit on
>  #define SET_PARM_BOOT_FLAGS_VALID_PERMANENT  0xC0 //boot flags data1 7 & 8 bit on 
>  
> -
> +#define SIZE_MAC                             18
> +#define SIZE_HOST_NETWORK_DATA               26

How did you decide on the size for SIZE_HOST_NETWORK_DATA?

> +#define SIZE_BOOT_OPTION                     SIZE_HOST_NETWORK_DATA
> +#define SIZE_PREFIX                          7
>  
>  // OpenBMC Chassis Manager dbus framework
>  const char  *chassis_bus_name      =  "org.openbmc.control.Chassis";
> @@ -233,14 +241,174 @@ struct get_sys_boot_options_t {
>  struct get_sys_boot_options_response_t {
>      uint8_t version;
>      uint8_t parm;
> -    uint8_t data[5];
> +    uint8_t data[SIZE_BOOT_OPTION];
>  }  __attribute__ ((packed));
>  
>  struct set_sys_boot_options_t {
>      uint8_t parameter;
> -    uint8_t data[8];
> +    uint8_t data[SIZE_BOOT_OPTION];
>  }  __attribute__ ((packed));
>  
> +struct host_network_config_t {
> +     string ipaddress;
> +     string prefix;
> +     string gateway;
> +     string macaddress;
> +     string isDHCP;

Is there a need to have these all as strings?

> +
> +     host_network_config_t():ipaddress(),prefix(),gateway(),macaddress() {}
> +};
> +
> +uint8_t getHostNetworkData(get_sys_boot_options_response_t *respptr)
> +{
> +    char *prop = NULL;
> +
> +    std::array<uint8_t, SIZE_BOOT_OPTION> respData{0x80,0x21, 0x70 ,0x62 ,0x21,0x00 ,0x01 ,0x06 ,0x04};

The "0x21, 0x70 ,0x62 ,0x21" cookie is a magic value that Petitboot uses
to recognise an override message it can read. Using it unconditionally
here means that openbmc would be hard-coded to only support Petitboot,
is that what we, or other users would expect?

> +
> +    int rc = dbus_get_property("network_config",&prop);
> +
> +    if (rc < 0) {
> +        fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");
> +        return rc;
> +    }
> +
> +    /* network_config property Value would be in the form of 
> +     * ipaddress=1.1.1.1,prefix=16,gateway=2.2.2.2,mac=11:22:33:44:55:66,dhcp=0
> +     */
> +
> +    /* Parsing the string and fill the hostconfig structure with the 
> +     * values */

If I'm reading this right the host_config structure is filled in
according to the "network_config" string, and then each field is copied
into respData - would it be simpler to skip the structure and just
fill in respData?

> +
> +    host_network_config_t host_config;
> +    string delimiter = ",";
> +
> +    size_t pos = 0;
> +    string token,name,value;
> +    string conf_str(prop);
> +    
> +    printf ("Configuration String[%s]\n ",conf_str.c_str());

Should we print this all the time, or just on debug?

> +
> +    while ( conf_str.length() > 0) {
> +
> +        pos = conf_str.find(delimiter);
> +
> +        //This condition is to extract the last 
> +        //Substring as we will not be having the delimeter 
> +        //at end. std::string::npos is -1
> +        
> +        if ( pos == std::string::npos )
> +        { 
> +           pos = conf_str.length();
> +        }
> +        token = conf_str.substr(0, pos);
> +        int pos1 = token.find("=");
> +
> +        name = token.substr(0,pos1);
> +        value = token.substr(pos1+1,pos);
> +
> +        if ( name == "ipaddress" )
> +            host_config.ipaddress = value;
> +        else if ( name == "prefix")
> +            host_config.prefix = value; 
> +        else if ( name == "gateway" )
> +            host_config.gateway = value;
> +        else if ( name == "mac" )
> +            host_config.macaddress = value;
> +        else if ( name == "dhcp" )
> +            host_config.isDHCP = value;
> +        
> +        conf_str.erase(0, pos + delimiter.length());
> +    }
> +
> +    //Starting from index 9 as 9 bytes are prefilled.
> +
> +    pos = 0; 
> +    delimiter = ":";
> +    uint8_t resp_byte = 0;
> +    uint8_t index = 9;
> +    
> +    while ((pos = host_config.macaddress.find(delimiter)) != std::string::npos) {
> +
> +        token = host_config.macaddress.substr(0, pos);
> +        resp_byte = strtoul(token.c_str(), NULL, 16);

Would be a good idea to check strtoul for errors

> +        memcpy((void*)&respData[index], &resp_byte, 1);

Don't need to cast to (void*)

> +        host_config.macaddress.erase(0, pos + delimiter.length());
> +        index++;
> +    }
> +
> +    resp_byte = strtoul(host_config.macaddress.c_str(), NULL, 16);
> +    memcpy((void*)&respData[index++], &resp_byte, 1);

Same

> +
> +    //Conevrt the dhcp,ipaddress,mask and gateway as hex number

Typo, Conevrt/Convert

> +    respData[index++]=0x00;
> +    sscanf(host_config.isDHCP.c_str(),"%02X",&respData[index++]);
> +
> +    inet_pton(AF_INET,host_config.ipaddress.c_str(),(void *)&respData[index]);
> +    index+=4;
> +
> +    sscanf(host_config.prefix.c_str(),"%02d",&respData[index++]);
> +    inet_pton(AF_INET,host_config.gateway.c_str(),(void *)&respData[index]);
> +    index+=4; 
> +    
> +    printf ("\n===Printing the IPMI Formatted Data========\n");

As above, debug message?

> +
> +    for (int j = 0;j<index;j++)

Spacing it out like this is easier to read:

    for (int j = 0; j < index; j++)

> +        printf("%02x ", respData[j]);
> +
> +    memcpy(respptr->data,respData.data(),SIZE_BOOT_OPTION);
> +    return 0;
> +}
> +
> +uint8_t setHostNetworkData(set_sys_boot_options_t * reqptr)
> +{
> +    string host_network_config;
> +    char mac[SIZE_MAC] = {0};
> +    char ipAddress[INET_ADDRSTRLEN] = {0};
> +    char gateway[INET_ADDRSTRLEN] = {0};
> +    char dhcp[SIZE_PREFIX] = {0};
> +    char prefix[SIZE_PREFIX] = {0};
> +    uint32_t cookie = 0;
> +
> +    memcpy(&cookie,(char *)&(reqptr->data[1]),sizeof(cookie));
> +    
> +    uint8_t index = 9;  
> +
> +    if ( cookie) {
> +     
> +            snprintf(mac, SIZE_MAC, "%02x:%02x:%02x:%02x:%02x:%02x",
> +            reqptr->data[index],
> +            reqptr->data[index+1],
> +            reqptr->data[index+2],
> +            reqptr->data[index+3],
> +            reqptr->data[index+4],
> +            reqptr->data[index+5]);
> +    
> +        snprintf(dhcp,SIZE_PREFIX, "%d", reqptr->data[index+7]);

*If* this is following the format used by Petitboot, is the order of
fields here correct? It looks like the size fields have been skipped.

> +
> +        snprintf(ipAddress, INET_ADDRSTRLEN, "%d.%d.%d.%d",
> +            reqptr->data[index+8], reqptr->data[index+9], reqptr->data[index+10], reqptr->data[index+11]);
> +
> +        snprintf(prefix,SIZE_PREFIX,"%d", reqptr->data[index+12]);

What is the prefix? Is it meant to be the subnet mask?

> +
> +        snprintf(gateway, INET_ADDRSTRLEN, "%d.%d.%d.%d",
> +            reqptr->data[index+13], reqptr->data[index+14], reqptr->data[index+15], reqptr->data[index+16]);
> +    }
> +
> +    host_network_config += "ipaddress="+string(ipAddress)+",prefix="+ \
> +                       string(prefix)+",gateway="+string(gateway)+\
> +                       ",mac="+string(mac)+",dhcp="+string(dhcp);
> +
> +    printf ("Network configuration changed: %s\n",host_network_config.c_str());
> +
> +    int r = dbus_set_property("network_config",host_network_config.c_str());
> +
> +    if (r < 0) {
> +        fprintf(stderr, "Dbus set property(network_config) failed for set_sys_boot_options.\n");
> +        r = IPMI_CC_UNSPECIFIED_ERROR;
> +    }
> +    return r;
> +}
> +
>  ipmi_ret_t ipmi_chassis_wildcard(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 
>                                ipmi_request_t request, ipmi_response_t response, 
>                                ipmi_data_len_t data_len, ipmi_context_t context)
> @@ -446,7 +614,18 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>          }
>  
>  
> -    } else {
> +    } else if ( reqptr->parameter == 0x61 ) {
> +       resp->parm      = 0x61;
> +       int ret = getHostNetworkData(resp);
> +       if (ret < 0) {
> +           fprintf(stderr, "getHostNetworkData failed for get_sys_boot_options.\n");
> +           rc = IPMI_CC_UNSPECIFIED_ERROR;
> +
> +       }else
> +          rc = IPMI_CC_OK;
> +    }
> +
> +    else {
>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
>      }
>  
> @@ -464,11 +643,10 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  {
>      ipmi_ret_t rc = IPMI_CC_OK;
>      char *s;
> -
> -    printf("IPMI SET_SYS_BOOT_OPTIONS\n");
> -
>      set_sys_boot_options_t *reqptr = (set_sys_boot_options_t *) request;
>  
> +    printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);
> +
>      // This IPMI command does not have any resposne data
>      *data_len = 0;
>  
> @@ -476,6 +654,7 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>       * Parameter #5 means boot flags. Please refer to 28.13 of ipmi doc.
>       * This is the only parameter used by petitboot.
>       */
> +

Extra line

>      if (reqptr->parameter == 5) {
>  
>          s = get_boot_option_by_ipmi(((reqptr->data[1] & 0x3C) >> 2));
> @@ -507,7 +686,16 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>              rc = IPMI_CC_UNSPECIFIED_ERROR;
>          }
>  
> -    } else {
> +    }
> +    else if ( reqptr->parameter == 0x61 ) {
> +       printf("IPMI SET_SYS_BOOT_OPTIONS reqptr->parameter =[%d]\n",reqptr->parameter);
> +       int ret = setHostNetworkData(reqptr);
> +       if (ret < 0) {
> +           fprintf(stderr, "setHostNetworkData failed for set_sys_boot_options.\n");
> +           rc = IPMI_CC_UNSPECIFIED_ERROR;
> +       }
> +    }      
> +    else {
>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
>          rc = IPMI_CC_PARM_NOT_SUPPORTED;
>      }
> -- 
> 2.8.4
> 
> 
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

Re: [PATCH phosphor-host-ipmid v5] Resolves openbmc/openbmc#267'

[Ratan K Gupta]

[-- Attachment #1: Type: text/html, Size: 19447 bytes --]

Re: [PATCH phosphor-host-ipmid v5] Resolves openbmc/openbmc#267'

[Samuel Mendoza-Jonas]

On Fri, Jun 17, 2016 at 11:48:52AM +0000, Ratan K Gupta wrote:
> Hi Sam,
>  
> Thanks for looking at the code.
>  
> Please find the response inline.
>  
> - What is the purpose of the host_network_config struct, and why is the
>   request parsed into a string before being stored? Why not just store
>   the original request as is?
>  
>   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.

Right, using the readable string for the REST interface is good, but in
getHostNetworkData() for example, it looks like you
	- Get the network_config property (the string)
	- Parse it into the host_network_config_t struct
	- Parse the struct into a hex string

What purpose does the host_network_config_t struct serve?

> 
> - Do you have tests to verify the correctness of the hex produced?
>   RG>>I verified 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.
>    
>      Is there a path where you think that Hex will not be correct? Please let
> me know so that I can look into it.

There was one part I wasn't sure about - please see the comments I made
inline orginally below:

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

> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc