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

[Christophe Fergeau]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20150810/51e134dd/attachment-0001.sig>