[Spice-devel,qxl] spiceqxl: Recognize the same set of boolean values as in xorg.conf.

Submitted by Francois Gouget on June 8, 2015, 3:48 p.m.

Details

Message ID alpine.DEB.2.11.1506081738510.31098@amboise
State New
Headers show

Not browsing as part of any series.

Commit Message

Francois Gouget June 8, 2015, 3:48 p.m.
Report an error if an invalid boolean value is used, just like for other options.
---

This seems simple enough and may avoid confusion.

 src/qxl_option_helpers.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/qxl_option_helpers.c b/src/qxl_option_helpers.c
index 8801b53..c9ad726 100644
--- a/src/qxl_option_helpers.c
+++ b/src/qxl_option_helpers.c
@@ -3,6 +3,7 @@ 
 #endif
 
 #include <stdlib.h>
+#include <strings.h>
 
 #include <xf86.h>
 #include <xf86Opt.h>
@@ -30,12 +31,24 @@  const char *get_str_option(OptionInfoPtr options, int option_index,
 int get_bool_option(OptionInfoPtr options, int option_index,
                      const char *env_name)
 {
-    if (getenv(env_name)) {
-        /* we don't support the whole range of boolean true and
-         * false values documented in man xorg.conf, just the c
-         * convention - 0 is false, anything else is true, so
-         * just like a number. */
-        return !!atoi(getenv(env_name));
+    const char* value = getenv(env_name);
+
+    if (!value) {
+        return options[option_index].value.bool;
+    }
+    if (strcmp(value, "1") == 0 ||
+        strcasecmp(value, "on") == 0 ||
+        strcasecmp(value, "true") == 0 ||
+        strcasecmp(value, "yes") == 0) {
+        return TRUE;
     }
-    return options[option_index].value.bool;
+    if (strcmp(value, "0") == 0 ||
+        strcasecmp(value, "off") == 0 ||
+        strcasecmp(value, "false") == 0 ||
+        strcasecmp(value, "no") == 0) {
+        return FALSE;
+    }
+
+    fprintf(stderr, "spice: invalid %s: %s\n", env_name, value);
+    exit(1);
 }

Comments

Hey,

Looks good to me, ACK.

There is a slight change of behaviour as before any != 0 values set for
the environment variable would return true, but I don't think this
matters

Christophe

On Mon, Jun 08, 2015 at 05:48:03PM +0200, Francois Gouget wrote:
> Report an error if an invalid boolean value is used, just like for other options.
> ---
> 
> This seems simple enough and may avoid confusion.
> 
>  src/qxl_option_helpers.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/src/qxl_option_helpers.c b/src/qxl_option_helpers.c
> index 8801b53..c9ad726 100644
> --- a/src/qxl_option_helpers.c
> +++ b/src/qxl_option_helpers.c
> @@ -3,6 +3,7 @@
>  #endif
>  
>  #include <stdlib.h>
> +#include <strings.h>
>  
>  #include <xf86.h>
>  #include <xf86Opt.h>
> @@ -30,12 +31,24 @@ const char *get_str_option(OptionInfoPtr options, int option_index,
>  int get_bool_option(OptionInfoPtr options, int option_index,
>                       const char *env_name)
>  {
> -    if (getenv(env_name)) {
> -        /* we don't support the whole range of boolean true and
> -         * false values documented in man xorg.conf, just the c
> -         * convention - 0 is false, anything else is true, so
> -         * just like a number. */
> -        return !!atoi(getenv(env_name));
> +    const char* value = getenv(env_name);
> +
> +    if (!value) {
> +        return options[option_index].value.bool;
> +    }
> +    if (strcmp(value, "1") == 0 ||
> +        strcasecmp(value, "on") == 0 ||
> +        strcasecmp(value, "true") == 0 ||
> +        strcasecmp(value, "yes") == 0) {
> +        return TRUE;
>      }
> -    return options[option_index].value.bool;
> +    if (strcmp(value, "0") == 0 ||
> +        strcasecmp(value, "off") == 0 ||
> +        strcasecmp(value, "false") == 0 ||
> +        strcasecmp(value, "no") == 0) {
> +        return FALSE;
> +    }
> +
> +    fprintf(stderr, "spice: invalid %s: %s\n", env_name, value);
> +    exit(1);
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Hey,
> 
> Looks good to me, ACK.
> 
> There is a slight change of behaviour as before any != 0 values set for
> the environment variable would return true, but I don't think this
> matters
> 
> Christophe
> 
> On Mon, Jun 08, 2015 at 05:48:03PM +0200, Francois Gouget wrote:
> > Report an error if an invalid boolean value is used, just like for other
> > options.
> > ---
> > 
> > This seems simple enough and may avoid confusion.
> > 
> >  src/qxl_option_helpers.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/qxl_option_helpers.c b/src/qxl_option_helpers.c
> > index 8801b53..c9ad726 100644
> > --- a/src/qxl_option_helpers.c
> > +++ b/src/qxl_option_helpers.c
> > @@ -3,6 +3,7 @@
> >  #endif
> >  
> >  #include <stdlib.h>
> > +#include <strings.h>
> >  
> >  #include <xf86.h>
> >  #include <xf86Opt.h>
> > @@ -30,12 +31,24 @@ const char *get_str_option(OptionInfoPtr options, int
> > option_index,
> >  int get_bool_option(OptionInfoPtr options, int option_index,
> >                       const char *env_name)
> >  {
> > -    if (getenv(env_name)) {
> > -        /* we don't support the whole range of boolean true and
> > -         * false values documented in man xorg.conf, just the c
> > -         * convention - 0 is false, anything else is true, so
> > -         * just like a number. */
> > -        return !!atoi(getenv(env_name));
> > +    const char* value = getenv(env_name);
> > +
> > +    if (!value) {
> > +        return options[option_index].value.bool;
> > +    }
> > +    if (strcmp(value, "1") == 0 ||
> > +        strcasecmp(value, "on") == 0 ||
> > +        strcasecmp(value, "true") == 0 ||
> > +        strcasecmp(value, "yes") == 0) {
> > +        return TRUE;
> >      }
> > -    return options[option_index].value.bool;
> > +    if (strcmp(value, "0") == 0 ||
> > +        strcasecmp(value, "off") == 0 ||
> > +        strcasecmp(value, "false") == 0 ||
> > +        strcasecmp(value, "no") == 0) {
> > +        return FALSE;
> > +    }
> > +
> > +    fprintf(stderr, "spice: invalid %s: %s\n", env_name, value);
> > +    exit(1);
> >  }
> > --
> > 2.1.4
> > 

Is it worth splitting the patch in two, one for extended values and the other for not accepting invalid values?

Frediano
On Mon, Aug 10, 2015 at 08:43:12AM -0400, Frediano Ziglio wrote:
> > 
> > Hey,
> > 
> > Looks good to me, ACK.
> > 
> > There is a slight change of behaviour as before any != 0 values set for
> > the environment variable would return true, but I don't think this
> > matters
> > 
> > Christophe
> > 
> > On Mon, Jun 08, 2015 at 05:48:03PM +0200, Francois Gouget wrote:
> > > Report an error if an invalid boolean value is used, just like for other
> > > options.
> > > ---
> > > 
> > > This seems simple enough and may avoid confusion.
> > > 
> > >  src/qxl_option_helpers.c | 27 ++++++++++++++++++++-------
> > >  1 file changed, 20 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/qxl_option_helpers.c b/src/qxl_option_helpers.c
> > > index 8801b53..c9ad726 100644
> > > --- a/src/qxl_option_helpers.c
> > > +++ b/src/qxl_option_helpers.c
> > > @@ -3,6 +3,7 @@
> > >  #endif
> > >  
> > >  #include <stdlib.h>
> > > +#include <strings.h>
> > >  
> > >  #include <xf86.h>
> > >  #include <xf86Opt.h>
> > > @@ -30,12 +31,24 @@ const char *get_str_option(OptionInfoPtr options, int
> > > option_index,
> > >  int get_bool_option(OptionInfoPtr options, int option_index,
> > >                       const char *env_name)
> > >  {
> > > -    if (getenv(env_name)) {
> > > -        /* we don't support the whole range of boolean true and
> > > -         * false values documented in man xorg.conf, just the c
> > > -         * convention - 0 is false, anything else is true, so
> > > -         * just like a number. */
> > > -        return !!atoi(getenv(env_name));
> > > +    const char* value = getenv(env_name);
> > > +
> > > +    if (!value) {
> > > +        return options[option_index].value.bool;
> > > +    }
> > > +    if (strcmp(value, "1") == 0 ||
> > > +        strcasecmp(value, "on") == 0 ||
> > > +        strcasecmp(value, "true") == 0 ||
> > > +        strcasecmp(value, "yes") == 0) {
> > > +        return TRUE;
> > >      }
> > > -    return options[option_index].value.bool;
> > > +    if (strcmp(value, "0") == 0 ||
> > > +        strcasecmp(value, "off") == 0 ||
> > > +        strcasecmp(value, "false") == 0 ||
> > > +        strcasecmp(value, "no") == 0) {
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    fprintf(stderr, "spice: invalid %s: %s\n", env_name, value);
> > > +    exit(1);
> > >  }
> > > --
> > > 2.1.4
> > > 
> 
> Is it worth splitting the patch in two, one for extended values and
> the other for not accepting invalid values?

This would indeed be slightly better, it's in my opinion acceptable as
part of this patch too (maybe this could be mentioned in the commit log
though).

Christophe