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

[Frediano Ziglio]

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