All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH phosphor-host-ipmid] Adding Boot Policy
@ 2016-04-04 11:30 OpenBMC Patches
  2016-04-04 11:30 ` OpenBMC Patches
  0 siblings, 1 reply; 5+ messages in thread
From: OpenBMC Patches @ 2016-04-04 11:30 UTC (permalink / raw)
  To: openbmc

Adding the boot policy support for get/Set system boot option.Policy can be onetime which is applicable to next boot only or 
Permanent which is applicable for all the future boots.

<!-- 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/83)
<!-- Reviewable:end -->


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

ratagupt (1):
  Adding Boot Policy

 chassishandler.C | 59 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 12 deletions(-)

-- 
2.7.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH phosphor-host-ipmid] Adding Boot Policy
  2016-04-04 11:30 [PATCH phosphor-host-ipmid] Adding Boot Policy OpenBMC Patches
@ 2016-04-04 11:30 ` OpenBMC Patches
  2016-04-05  1:31   ` Cyril Bur
                     ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: OpenBMC Patches @ 2016-04-04 11:30 UTC (permalink / raw)
  To: openbmc; +Cc: ratagupt

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

---
 chassishandler.C | 59 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 12 deletions(-)

diff --git a/chassishandler.C b/chassishandler.C
index fca3c79..1a5508f 100644
--- a/chassishandler.C
+++ b/chassishandler.C
@@ -92,7 +92,7 @@ finish:
     return r;
 }
 
-int dbus_get_property(char **buf)
+int dbus_get_property(const char *name,char **buf)
 {
     sd_bus_error error = SD_BUS_ERROR_NULL;
     sd_bus_message *m = NULL;
@@ -128,7 +128,7 @@ int dbus_get_property(char **buf)
                            &m,                                         /* return message on success */
                            "ss",                                       /* input signature */
                            host_intf_name,                             /* first argument */
-                           "boot_flags");                              /* second argument */
+                           name);                                      /* second argument */
 
     if (r < 0) {
         fprintf(stderr, "Failed to issue method call: %s\n", error.message);
@@ -161,7 +161,7 @@ finish:
     return r;
 }
 
-int dbus_set_property(const char *buf)
+int dbus_set_property(const char * name,const char *value)
 {
     sd_bus_error error = SD_BUS_ERROR_NULL;
     sd_bus_message *m = NULL;
@@ -196,16 +196,16 @@ int dbus_set_property(const char *buf)
                            &m,                                         /* return message on success */
                            "ssv",                                      /* input signature */
                            host_intf_name,                             /* first argument */
-                           "boot_flags",                               /* second argument */
+                           name,                                       /* second argument */
                            "s",                                        /* third argument */
-                           buf);                                       /* fourth argument */
+                           value);                                       /* fourth argument */
 
     if (r < 0) {
         fprintf(stderr, "Failed to issue method call: %s\n", error.message);
         goto finish;
     }
 
-    printf("IPMID boot option property set: {%s}.\n", buf);
+    printf("IPMID boot option property set: {%s}.\n", value);
 
 finish:
     sd_bus_error_free(&error);
@@ -374,7 +374,8 @@ char* get_boot_option_by_ipmi(uint8_t p) {
 }
 
 #define SET_PARM_VERSION 1
-#define SET_PARM_BOOT_FLAGS_VALID   0x80
+#define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME   0x80
+#define SET_PARM_BOOT_FLAGS_VALID_PERMANENT  0xC0
 
 ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 
                               ipmi_request_t request, ipmi_response_t response, 
@@ -391,7 +392,7 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
     memset(resp,0,sizeof(*resp));
     resp->version   = SET_PARM_VERSION;
     resp->parm      = 5;
-    resp->data[0]   = SET_PARM_BOOT_FLAGS_VALID;
+    resp->data[0]   = SET_PARM_BOOT_FLAGS_VALID_ONE_TIME;
 
     *data_len = sizeof(*resp);
 
@@ -401,10 +402,11 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
      */
     if (reqptr->parameter == 5) {
 
-        int r = dbus_get_property(&p);
+        /* Get the boot device */
+        int r = dbus_get_property("boot_flags",&p);
 
         if (r < 0) {
-            fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
+            fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");
             rc = IPMI_CC_UNSPECIFIED_ERROR;
 
         } else {
@@ -412,8 +414,31 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
             s = get_ipmi_boot_option(p);
             resp->data[1] = (s << 2);
             rc = IPMI_CC_OK;
+
+        }
+
+        if (p)
+        {
+          free(p);
+          p = NULL;
+        }
+
+        /* Get the boot policy */
+        r = dbus_get_property("boot_policy",&p);
+
+        if (r < 0) {
+            fprintf(stderr, "Dbus get property(boot_policy) failed for get_sys_boot_options.\n");
+            rc = IPMI_CC_UNSPECIFIED_ERROR;
+
+        } else {
+
+            printf("BootPolicy is[%s]", p); 
+            resp->data[0] = (strcmp(p,"ONETIME")==0)?SET_PARM_BOOT_FLAGS_VALID_ONE_TIME:SET_PARM_BOOT_FLAGS_VALID_PERMANENT;
+            rc = IPMI_CC_OK;
+
         }
 
+
     } else {
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
     }
@@ -455,13 +480,23 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
 
         } else {
 
-            int r = dbus_set_property(s);
+            int r = dbus_set_property("boot_flags",s);
 
             if (r < 0) {
-                fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");
+                fprintf(stderr, "Dbus set property(boot_flags) failed for set_sys_boot_options.\n");
                 rc = IPMI_CC_UNSPECIFIED_ERROR;
             }
         }
+      
+        /* setting the boot policy */
+        s= (char *)(((reqptr->data[0] & 0x40) == 0x40) ?"PERMANENT":"ONETIME");
+        printf ( "\nBoot Policy is %s",s); 
+        int r = dbus_set_property("boot_policy",s);
+
+        if (r < 0) {
+            fprintf(stderr, "Dbus set property(boot_policy) failed for set_sys_boot_options.\n");
+            rc = IPMI_CC_UNSPECIFIED_ERROR;
+            }
 
     } else {
         fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
-- 
2.7.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH phosphor-host-ipmid] Adding Boot Policy
  2016-04-04 11:30 ` OpenBMC Patches
@ 2016-04-05  1:31   ` Cyril Bur
  2016-04-18  8:38   ` Ratan K Gupta
       [not found]   ` <201604180839.u3I8d2r333685516@d28relay01.in.ibm.com>
  2 siblings, 0 replies; 5+ messages in thread
From: Cyril Bur @ 2016-04-05  1:31 UTC (permalink / raw)
  To: ratagupt; +Cc: OpenBMC Patches, openbmc

On Mon,  4 Apr 2016 06:30:36 -0500
OpenBMC Patches <openbmc-patches@stwcx.xyz> wrote:

Hi,

Thanks for the contribution. I have some concerns, most are summarised here:
https://github.com/openbmc/docs/blob/master/contributing.md

You'll want to note
https://github.com/openbmc/docs/blob/master/contributing.md#coding-style and
https://github.com/openbmc/docs/blob/master/contributing.md#submitting-changes

I've added further comments inline.

Thanks,

Cyril

> From: ratagupt <ratagupt@in.ibm.com>
> 
> ---
>  chassishandler.C | 59 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/chassishandler.C b/chassishandler.C
> index fca3c79..1a5508f 100644
> --- a/chassishandler.C
> +++ b/chassishandler.C
> @@ -92,7 +92,7 @@ finish:
>      return r;
>  }
>  
> -int dbus_get_property(char **buf)
> +int dbus_get_property(const char *name,char **buf)

The style guide recommends spaces between parameters.

>  {
>      sd_bus_error error = SD_BUS_ERROR_NULL;
>      sd_bus_message *m = NULL;
> @@ -128,7 +128,7 @@ int dbus_get_property(char **buf)
>                             &m,                                         /* return message on success */
>                             "ss",                                       /* input signature */
>                             host_intf_name,                             /* first argument */
> -                           "boot_flags");                              /* second argument */
> +                           name);                                      /* second argument */
>  
>      if (r < 0) {
>          fprintf(stderr, "Failed to issue method call: %s\n", error.message);
> @@ -161,7 +161,7 @@ finish:
>      return r;
>  }
>  
> -int dbus_set_property(const char *buf)
> +int dbus_set_property(const char * name,const char *value)
>  {
>      sd_bus_error error = SD_BUS_ERROR_NULL;
>      sd_bus_message *m = NULL;
> @@ -196,16 +196,16 @@ int dbus_set_property(const char *buf)
>                             &m,                                         /* return message on success */
>                             "ssv",                                      /* input signature */
>                             host_intf_name,                             /* first argument */
> -                           "boot_flags",                               /* second argument */
> +                           name,                                       /* second argument */
>                             "s",                                        /* third argument */
> -                           buf);                                       /* fourth argument */
> +                           value);                                       /* fourth argument */

I'm not really sure why the /* {nth} argument */ comments but since they're
there please try to keep them aligned...

>  
>      if (r < 0) {
>          fprintf(stderr, "Failed to issue method call: %s\n", error.message);
>          goto finish;
>      }
>  
> -    printf("IPMID boot option property set: {%s}.\n", buf);
> +    printf("IPMID boot option property set: {%s}.\n", value);
>  
>  finish:
>      sd_bus_error_free(&error);
> @@ -374,7 +374,8 @@ char* get_boot_option_by_ipmi(uint8_t p) {
>  }
>  
>  #define SET_PARM_VERSION 1
> -#define SET_PARM_BOOT_FLAGS_VALID   0x80
> +#define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME   0x80
> +#define SET_PARM_BOOT_FLAGS_VALID_PERMANENT  0xC0
>  
>  ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 
>                                ipmi_request_t request, ipmi_response_t response, 
> @@ -391,7 +392,7 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>      memset(resp,0,sizeof(*resp));
>      resp->version   = SET_PARM_VERSION;
>      resp->parm      = 5;
> -    resp->data[0]   = SET_PARM_BOOT_FLAGS_VALID;
> +    resp->data[0]   = SET_PARM_BOOT_FLAGS_VALID_ONE_TIME;
>  
>      *data_len = sizeof(*resp);
>  
> @@ -401,10 +402,11 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>       */
>      if (reqptr->parameter == 5) {
>  
> -        int r = dbus_get_property(&p);
> +        /* Get the boot device */
> +        int r = dbus_get_property("boot_flags",&p);
>  
>          if (r < 0) {
> -            fprintf(stderr, "Dbus get property failed for get_sys_boot_options.\n");
> +            fprintf(stderr, "Dbus get property(boot_flags) failed for get_sys_boot_options.\n");
>              rc = IPMI_CC_UNSPECIFIED_ERROR;
>  
>          } else {
> @@ -412,8 +414,31 @@ ipmi_ret_t ipmi_chassis_get_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>              s = get_ipmi_boot_option(p);
>              resp->data[1] = (s << 2);
>              rc = IPMI_CC_OK;
> +
> +        }
> +
> +        if (p)
> +        {
> +          free(p);
> +          p = NULL;
> +        }

Just fyi free() will happily accept NULL as an argument and simply do nothing.

> +
> +        /* Get the boot policy */
> +        r = dbus_get_property("boot_policy",&p);
> +
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus get property(boot_policy) failed for get_sys_boot_options.\n");
> +            rc = IPMI_CC_UNSPECIFIED_ERROR;
> +
> +        } else {
> +
> +            printf("BootPolicy is[%s]", p); 
> +            resp->data[0] = (strcmp(p,"ONETIME")==0)?SET_PARM_BOOT_FLAGS_VALID_ONE_TIME:SET_PARM_BOOT_FLAGS_VALID_PERMANENT;

Wow, could you please make use of the spacebar and perhaps the enter key?

Also, just to be safe it would be best of use strncmp.
For example strncmp(p, "ONETIME", strlen("ONETIME"))

> +            rc = IPMI_CC_OK;
> +
>          }
>  
> +
>      } else {
>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
>      }
> @@ -455,13 +480,23 @@ ipmi_ret_t ipmi_chassis_set_sys_boot_options(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
>  
>          } else {
>  
> -            int r = dbus_set_property(s);
> +            int r = dbus_set_property("boot_flags",s);
>  
>              if (r < 0) {
> -                fprintf(stderr, "Dbus set property failed for set_sys_boot_options.\n");
> +                fprintf(stderr, "Dbus set property(boot_flags) failed for set_sys_boot_options.\n");
>                  rc = IPMI_CC_UNSPECIFIED_ERROR;
>              }
>          }
> +      
> +        /* setting the boot policy */
> +        s= (char *)(((reqptr->data[0] & 0x40) == 0x40) ?"PERMANENT":"ONETIME");

Firstly if you declare s as a 'const char *' (which by the looks of things it
probably should be anyway...) then you won't need the cast. Why introduce 0x40?
Wouldn't it be cleaner to just do

if ((reqptr->data[0] & SET_PARM_BOOT_FLAGS_VALID_PERMANENT) ==
                      SET_PARM_BOOT_FLAGS_VALID_PERMANENT)
	s = "PERMANENT";
else
	s = "ONETIME";


> +        printf ( "\nBoot Policy is %s",s); 
> +        int r = dbus_set_property("boot_policy",s);
> +
> +        if (r < 0) {
> +            fprintf(stderr, "Dbus set property(boot_policy) failed for set_sys_boot_options.\n");
> +            rc = IPMI_CC_UNSPECIFIED_ERROR;
> +            }
>  
>      } else {
>          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH phosphor-host-ipmid] Adding Boot Policy
  2016-04-04 11:30 ` OpenBMC Patches
  2016-04-05  1:31   ` Cyril Bur
@ 2016-04-18  8:38   ` Ratan K Gupta
       [not found]   ` <201604180839.u3I8d2r333685516@d28relay01.in.ibm.com>
  2 siblings, 0 replies; 5+ messages in thread
From: Ratan K Gupta @ 2016-04-18  8:38 UTC (permalink / raw)
  To: cyrilbur; +Cc: openbmc-patches, openbmc

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH phosphor-host-ipmid] Adding Boot Policy
       [not found]   ` <201604180839.u3I8d2r333685516@d28relay01.in.ibm.com>
@ 2016-04-18  8:48     ` Cyril Bur
  0 siblings, 0 replies; 5+ messages in thread
From: Cyril Bur @ 2016-04-18  8:48 UTC (permalink / raw)
  To: Ratan K Gupta; +Cc: openbmc-patches, openbmc

On Mon, 18 Apr 2016 08:38:58 +0000
"Ratan K Gupta" <ratagupt@in.ibm.com> wrote:

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

Yes I understand that, which is why I added the 
'== SET_PARM_BOOT_FLAGS_VALID_PERMANENT' so that the condition is only true if
both bits 6 and 7 are set.

For example: If only bit 7 is set then (reqptr->data[0] &
SET_PARM_BOOT_FLAGS_VALID_PERMANENT) will not ==
SET_PARM_BOOT_FLAGS_VALID_PERMANENT

> Anyways I have created the macro for better readability.
> 
> > +        printf ( "\nBoot Policy is %s",s);
> > +        int r = dbus_set_property("boot_policy",s);
> > +
> > +        if (r < 0) {
> > +            fprintf(stderr, "Dbus set property(boot_policy) failed for set_sys_boot_options.\n");
> > +            rc = IPMI_CC_UNSPECIFIED_ERROR;
> > +            }
> >  
> >      } else {
> >          fprintf(stderr, "Unsupported parameter 0x%x\n", reqptr->parameter);
>  
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-04-18  8:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-04 11:30 [PATCH phosphor-host-ipmid] Adding Boot Policy OpenBMC Patches
2016-04-04 11:30 ` OpenBMC Patches
2016-04-05  1:31   ` Cyril Bur
2016-04-18  8:38   ` Ratan K Gupta
     [not found]   ` <201604180839.u3I8d2r333685516@d28relay01.in.ibm.com>
2016-04-18  8:48     ` Cyril Bur

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.