<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Hi Cyril,</div>
<div dir="ltr" > </div>
<div dir="ltr" >Please find my comments inline.</div>
<div dir="ltr" > </div>
<div dir="ltr" >Regards</div>
<div dir="ltr" >Ratan</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 Patches <openbmc-patches@stwcx.xyz>, openbmc@lists.ozlabs.org<br>Subject: Re: [PATCH phosphor-host-ipmid] Adding Boot Policy<br>Date: Tue, Apr 5, 2016 7:03 AM<br>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >On Mon, 4 Apr 2016 06:30:36 -0500<br>OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:<br><br>Hi,<br><br>Thanks for the contribution. I have some concerns, most are summarised here:<br><a href="https://github.com/openbmc/docs/blob/master/contributing.md" target="_blank" >https://github.com/openbmc/docs/blob/master/contributing.md</a><br><br>You'll want to note<br><a href="https://github.com/openbmc/docs/blob/master/contributing.md#coding-style" target="_blank" >https://github.com/openbmc/docs/blob/master/contributing.md#coding-style</a> and<br><a href="https://github.com/openbmc/docs/blob/master/contributing.md#submitting-changes" target="_blank" >https://github.com/openbmc/docs/blob/master/contributing.md#submitting-changes</a><br><br>I've added further comments inline.<br><br>Thanks,<br><br>Cyril<br><br>> From: ratagupt <ratagupt@in.ibm.com><br>><br>> ---<br>> chassishandler.C | 59 ++++++++++++++++++++++++++++++++++++++++++++------------<br>> 1 file changed, 47 insertions(+), 12 deletions(-)<br>><br>> diff --git a/chassishandler.C b/chassishandler.C<br>> index fca3c79..1a5508f 100644<br>> --- a/chassishandler.C<br>> +++ b/chassishandler.C<br>> @@ -92,7 +92,7 @@ finish:<br>> return r;<br>> }<br>> <br>> -int dbus_get_property(char **buf)<br>> +int dbus_get_property(const char *name,char **buf)<br><br>The style guide recommends spaces between parameters.</font></div>
<div> </div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>>RG:- Done<br><br>> {<br>> sd_bus_error error = SD_BUS_ERROR_NULL;<br>> sd_bus_message *m = NULL;<br>> @@ -128,7 +128,7 @@ int dbus_get_property(char **buf)<br>> &m, /* return message on success */<br>> "ss", /* input signature */<br>> host_intf_name, /* first argument */<br>> - "boot_flags"); /* second argument */<br>> + name); /* second argument */<br>> <br>> if (r < 0) {<br>> fprintf(stderr, "Failed to issue method call: %s\n", error.message);<br>> @@ -161,7 +161,7 @@ finish:<br>> return r;<br>> }<br>> <br>> -int dbus_set_property(const char *buf)<br>> +int dbus_set_property(const char * name,const char *value)<br>> {<br>> sd_bus_error error = SD_BUS_ERROR_NULL;<br>> sd_bus_message *m = NULL;<br>> @@ -196,16 +196,16 @@ int dbus_set_property(const char *buf)<br>> &m, /* return message on success */<br>> "ssv", /* input signature */<br>> host_intf_name, /* first argument */<br>> - "boot_flags", /* second argument */<br>> + name, /* second argument */<br>> "s", /* third argument */<br>> - buf); /* fourth argument */<br>> + value); /* fourth argument */<br><br>I'm not really sure why the /* {nth} argument */ comments but since they're<br>there please try to keep them aligned...<br>>>>>RG:Done<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>> + printf("IPMID boot option property set: {%s}.\n", value);<br>> <br>> finish:<br>> sd_bus_error_free(&error);<br>> @@ -374,7 +374,8 @@ char* get_boot_option_by_ipmi(uint8_t p) {<br>> }<br>> <br>> #define SET_PARM_VERSION 1<br>> -#define SET_PARM_BOOT_FLAGS_VALID 0x80<br>> +#define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME 0x80<br>> +#define SET_PARM_BOOT_FLAGS_VALID_PERMANENT 0xC0<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>> @@ -391,7 +392,7 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> memset(resp,0,sizeof(*resp));<br>> resp->version = SET_PARM_VERSION;<br>> resp->parm = 5;<br>> - resp->data[0] = SET_PARM_BOOT_FLAGS_VALID;<br>> + resp->data[0] = SET_PARM_BOOT_FLAGS_VALID_ONE_TIME;<br>> <br>> *data_len = sizeof(*resp);<br>> <br>> @@ -401,10 +402,11 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> */<br>> if (reqptr->parameter == 5) {<br>> <br>> - int r = dbus_get_property(&p);<br>> + /* Get the boot device */<br>> + int r = dbus_get_property("boot_flags",&p);<br>> <br>> if (r < 0) {<br>> - fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");<br>> + fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");<br>> rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> <br>> } else {<br>> @@ -412,8 +414,31 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> s = get_ipmi_boot_option(p);<br>> resp->data[1] = (s << 2);<br>> rc = IPMI_CC_OK;<br>> +<br>> + }<br>> +<br>> + if (p)<br>> + {<br>> + free(p);<br>> + p = NULL;<br>> + }<br><br>Just fyi free() will happily accept NULL as an argument and simply do nothing.<br>>>>>RG:- I was following the approach which was there before.<br>> +<br>> + /* Get the boot policy */<br>> + r = dbus_get_property("boot_policy",&p);<br>> +<br>> + if (r < 0) {<br>> + fprintf(stderr, "Dbus get property(boot_policy) failed for get_sys_boot_options.\n");<br>> + rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> +<br>> + } else {<br>> +<br>> + printf("BootPolicy is[%s]", p);<br>> + resp->data[0] = (strcmp(p,"ONETIME")==0)?SET_PARM_BOOT_FLAGS_VALID_ONE_TIME:SET_PARM_BOOT_FLAGS_VALID_PERMANENT;<br><br>Wow, could you please make use of the spacebar and perhaps the enter key?</font></div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >RG:>>>>>>Done<br><br>Also, just to be safe it would be best of use strncmp.<br>For example strncmp(p, "ONETIME", strlen("ONETIME"))<br><br>> + rc = IPMI_CC_OK;<br>> +<br>> }<br>> <br>> +<br>> } else {<br>> fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);<br>> }<br>> @@ -455,13 +480,23 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,<br>> <br>> } else {<br>> <br>> - int r = dbus_set_property(s);<br>> + int r = dbus_set_property("boot_flags",s);<br>> <br>> if (r < 0) {<br>> - fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");<br>> + fprintf(stderr, "Dbus set property(boot_flags) failed for set_sys_boot_options.\n");<br>> rc = IPMI_CC_UNSPECIFIED_ERROR;<br>> }<br>> }<br>> + <br>> + /* setting the boot policy */<br>> + s= (char *)(((reqptr->data[0] & 0x40) == 0x40) ?"PERMANENT":"ONETIME");<br><br>Firstly if you declare s as a 'const char *' (which by the looks of things it<br>probably should be anyway...) then you won't need the cast. Why introduce 0x40?<br>Wouldn't it be cleaner to just do<br><br>if ((reqptr->data[0] & SET_PARM_BOOT_FLAGS_VALID_PERMANENT) ==<br> SET_PARM_BOOT_FLAGS_VALID_PERMANENT)<br>s = "PERMANENT";<br>else<br>s = "ONETIME";</font></div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >>>>>>>>RG: would not be able to check with SET_PARM_BOOT_FLAGS_VALID_PERMANENT as by doing this it checks for the two bits(6 and 7).But with the 0x40,We are only checking the one bit(6th bit).</font></div>
<div><font face="Default Monospace,Courier New,Courier,monospace" size="2" >Anyways I have created the macro for better readability.</font></div>
<div><br><font face="Default Monospace,Courier New,Courier,monospace" size="2" >> + printf ( "\nBoot Policy is %s",s);<br>> + int r = dbus_set_property("boot_policy",s);<br>> +<br>> + if (r < 0) {<br>> + fprintf(stderr, "Dbus set property(boot_policy) 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);</font></div></blockquote>
<div dir="ltr" > </div></div><BR>