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

2015-08-10 Thread 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


pgpZYuRcGRU6W.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2015-08-10 Thread Christophe Fergeau 
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


pgpL9FnSo70wk.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

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

2015-06-08 Thread Francois Gouget 
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