<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>