<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Thanks Cyril for your comment. </div>
<div dir="ltr" > </div>
<div dir="ltr" >I addressed most of your comments in my latest updates to my pull request.</div>
<div dir="ltr" > </div>
<div dir="ltr" >More answers as below, starting with ">>>> [pengfei]: "</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" 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: 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 v3] Add get/set ipmid command support with correct DBUS property handling.<br>Date: Mon, Jan 4, 2016 2:40 PM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Tue, 22 Dec 2015 20:30:27 -0600<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>> From: shgoupf <shgoupf@cn.ibm.com><br>><br><br>Hi shgoupf,<br><br>The patch needs work, unfortunately it appears there are some problems you've<br>inherited here but lets not continue them.<br><br>I've outlined some concerns below.<br><br>> 1) Two methods to handle the dbus property set and get:<br>> a) dbus_set_property()<br>> b) dbus_get_property()<br>> 2) The property is stored as a 10 character strings which representsd 5-byte information.<br>> 3) ipmid set method is registered and implemented since petitboot will use it to clear the boot options.<br>> 4) 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>> 5) Error code are properly handled.<br>> 6) 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>> chassishandler.C | 320 ++++++++++++++++++++++++++++++++++++++++++++++++++++---<br>> chassishandler.h | 17 +++<br>> 2 files changed, 325 insertions(+), 12 deletions(-)<br>><br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index 1389db9..d68b5a8 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -9,6 +9,205 @@ 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>> +// Host settings in dbus<br>> +// Service name should be referenced by connection name got via object mapper<br>> +// const char *settings_service_name = "org.openbmc.settings.Host";<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>> +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>> + if (r < 0) {<br>> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));<br>> + goto finish;<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>Multiline comments should use the multiline comment style /* */, so:<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> */</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Thanks. I have updated the comment style in my latest pull request.<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>> + // 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>So how sure are you that what you're doing now is robust?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: For now, per my testing, the result is correct. So I believe we can revisit this later when the issue is addressed.<br><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>Is there a reason you're sending commented code?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Just want to preserve the code that should be resumed when the issued is addressed.<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>> + 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>> + 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>> + 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>> + if (r < 0) {<br>> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));<br>> + goto finish;<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>> + 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>> + // The output should be parsed exactly the same as the output formatting<br>> + // specified.<br>> + r = sd_bus_message_read(m, "v", "s", &temp_buf);<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>> + memcpy(buf, temp_buf, 2*NUM_RETURN_BYTES_OF_GET_USED + 1);<br><br>This isn't the only place you do this 2*value + 1, I also don't see a place<br>you've used NUM_RETURN_BYTES_OF_GET_USED do you just want to make the #define<br>11? What are you trying to do?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: comments added in the code to explain this. Quoted as below:</font></div>
<div><table data-tab-size="8" > <tbody> <tr> <td>+ /*</td> </tr> <tr> <td> </td> <td data-line-number="420" id="diff-f609493af6d78ff9bfaa4742d40f1ca5R420" > </td> <td>+ * To represent a hex in string, e.g., "A0A0", which represents two bytes</td> </tr> <tr> <td> </td> <td data-line-number="421" id="diff-f609493af6d78ff9bfaa4742d40f1ca5R421" > </td> <td>+ * in the hex, but requires 5 bytes to store it as string, i.e., 4</td> </tr> <tr> <td> </td> <td data-line-number="422" id="diff-f609493af6d78ff9bfaa4742d40f1ca5R422" > </td> <td>+ * characters to store the "A0A0" and 1 additional space for "\0".</td> </tr> <tr> <td> </td> <td data-line-number="423" id="diff-f609493af6d78ff9bfaa4742d40f1ca5R423" > </td> <td>+ * Thereby we have 2 * <number of bytes> + 1 for the string buffer.</td> </tr> <tr> <td> </td> <td data-line-number="424" id="diff-f609493af6d78ff9bfaa4742d40f1ca5R424" > </td> <td>+ */</td> </tr> </tbody></table>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> +<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>> + if (r < 0) {<br>> + fprintf(stderr, "Failed to get connection, return value: %d.\n", r);<br>> + goto finish;<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>> + if (r < 0) {<br>> + fprintf(stderr, "Failed to connect to system bus: %s\n", strerror(-r));<br>> + goto finish;<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>> + 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>> + 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>> @@ -17,8 +216,25 @@ 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>> +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>Ok so I'm opposed to casting an array into a struct however, after having<br>read part 28.12 - how can you be sure that the data portion is going to be 8<br>bytes? It looks like you could get zero data...<br><br>"Passing 0-bytes of parameter data allows the parameter valid bit to be<br>changed without affecting the present parameter setting. "<br><br>I'm worried.</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: currently this patch only supports petitboot, as petitboot only uses 8 bytes, I believe it's fine to do this.<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>> {<br>> printf("Handling CHASSIS WILDCARD Netfn:[0x%X], Cmd:[0x%X]\n",netfn, cmd);<br>> @@ -48,7 +264,7 @@ int ipmi_chassis_power_control(const char *method)<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_object_name, // Object path<br>> chassis_intf_name, // Interface name<br>> method, // Method to be called<br>> &bus_error, // object to return error<br>> @@ -73,8 +289,8 @@ int ipmi_chassis_power_control(const char *method)<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_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>> @@ -105,8 +321,8 @@ ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<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_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>> @@ -116,23 +332,100 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> <br>> get_sys_boot_options_t *reqptr = (get_sys_boot_options_t*) request;<br>> <br>> - // TODO Return default values to OPAL until dbus interface is available<br>> + char* buf = (char*)malloc(NUM_RETURN_BYTES_OF_GET*2 + 1);<br><br>Why are you mallocing buf, whats wrong with having it on the stack?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>See my previous comment about the 2*value + 1, where you've used it twice with<br>2* +1 and once without, what's going on?<br>> +<br>> + if (buf == NULL) {<br>> + fprintf(stderr, "Malloc failed for get_sys_boot_options.\n");<br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> +<br>> + // Init the buffer to all zero.<br>> + memset(buf, 0, NUM_RETURN_BYTES_OF_GET*2 + 1);<br>> +<br><br>Might I suggest you use calloc(1, <size>); rather than malloc if you want<br>zeroed memory?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull 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>> + if (reqptr->parameter == 5)<br>> + {<br>> + int r = dbus_get_property(buf);<br>> +<br>> + if (r < 0) {<br>> + fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");<br><br>If I'm not mistaken you'll have a leak of buf here?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> +<br>> + uint64_t return_value;<br>> + sscanf(buf, "%llX", &return_value);<br><br>atoll()?</font></div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: I believe sscanf works fine so no need to change it.<br><br>> +<br>> + *data_len = NUM_RETURN_BYTES_OF_GET;<br>> + // TODO: last 3 bytes<br>> + // (NUM_RETURN_BYTES_OF_GET - NUM_RETURN_BYTES_OF_GET_USED) is meanlingless<br>> + memcpy(response, (uint8_t*)(&return_value), *data_len);<br>> + free(buf);<br>> + }<br>> + else<br>> + {<br>> + *data_len = NUM_RETURN_BYTES_OF_GET;<br>> + // 0x80: parameter not supported<br>> + buf[0] = 0x80;<br><br>#define?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>> + memcpy(response, buf, *data_len);<br>> + free(buf);<br>> + fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> + return IPMI_CC_PARM_NOT_SUPPORTED;<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", *data_len);<br>> +<br>> + set_sys_boot_options_t *reqptr = (set_sys_boot_options_t*) request;<br>> +<br>> + char* output_buf = (char*)malloc(NUM_RETURN_BYTES_OF_SET);<br>> +<br>> + if (output_buf == NULL) {<br>> + fprintf(stderr, "Malloc failed for set_sys_boot_options.\n");<br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> <br><br>You didn't zero the memory here? get/sets should probably do the same in this<br>respect.</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<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>> + char input_buf[NUM_INPUT_BYTES_OF_SET*2 + 1];<br><br>Gah those odd maths again?<br><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><br>Leak of output_buf?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>> + return IPMI_OUT_OF_SPACE;<br>> + }<br>> +<br>> + *data_len = NUM_RETURN_BYTES_OF_SET;<br>> + // 0x0: return code OK.<br>> + output_buf[0] = 0x0;<br><br>#define?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>> + memcpy(response, output_buf, *data_len);<br>> + free(output_buf);<br>> }<br>> else<br>> {<br>> + // 0x80: parameter not supported<br>> + output_buf[0] = 0x80;<br><br>#define?</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: Yes, good point. Updated in the latest pull request.<br><br>> + memcpy(response, output_buf, *data_len);<br>> + free(output_buf);<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>> <br>> return rc;<br>> }<br>> <br>> +<br>> void register_netfn_chassis_functions()<br>> {<br>> printf("Registering NetFn:[0x%X], Cmd:[0x%X]\n",NETFUN_CHASSIS, IPMI_CMD_WILDCARD);<br>> @@ -141,6 +434,9 @@ void register_netfn_chassis_functions()<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_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>> diff --git a/chassishandler.h b/chassishandler.h<br>> index 1a26411..4676667 100644<br>> --- a/chassishandler.h<br>> +++ b/chassishandler.h<br>> @@ -3,12 +3,22 @@<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>Just so I'm understanding correctly, you're writing an IPMI daemon here? I'm<br>not sure you should be noting quirks of petitboot, you should be doing what the<br>IPMI spec says</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>> [pengfei]: currently this patch only supports petitboot, as petitboot only uses 8 bytes, I believe it's fine to do this.<br><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>> @@ -18,6 +28,13 @@ enum ipmi_chassis_return_codes<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>> {</font></div></div></blockquote>
<div dir="ltr" > </div></div><BR>