[PATCH 2/5] libxl: add xlu_cfg_get_type function

[PATCH 2/5] libxl: add xlu_cfg_get_type function

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

As cpuid= can be used with two syntaxes (one as a list, the other as a
string), we need to distinguish them without an error message. Introduce 
a helper function to detect the type of entry used before issuing a warning.

Signed-off-by: Andre Przywara

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: 0002-libxl-add-xlu_cfg_get_type-function.patch --]
[-- Type: text/x-patch, Size: 2289 bytes --]

>From ae2a1dda7829a38bb891f560d9fc458a4d8c9fba Mon Sep 17 00:00:00 2001
From: Andre Przywara <andre.przywara@amd.com>
Date: Wed, 1 Sep 2010 14:11:54 +0200
Subject: [PATCH 2/5] libxl: add xlu_cfg_get_type function

cpuid= can be used with two syntaxes, one as a list, the other as a
string. Introduce a helper function to detect the type of entry used
before issuing a warning.

Signed-off-by: Andre Przywara
---
 tools/libxl/libxlu_cfg.c |   18 ++++++++++++++++++
 tools/libxl/libxlutil.h  |    6 ++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 09a7c71..b9018d5 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -124,6 +124,24 @@ static XLU_ConfigSetting *find(const XLU_Config *cfg, const char *n) {
     return 0;
 }
 
+int xlu_cfg_get_type(const XLU_Config *cfg, const char *n)
+{
+    XLU_ConfigSetting *set;
+    char *endptr;
+    long l;
+
+    set = find(cfg, n);
+    if (set == NULL)
+        return XLU_CFG_NOTFOUND;
+    if (set->avalues > 1)
+        return XLU_CFG_LIST;
+    errno = 0;
+    l = strtol(set->values[0], &endptr, 0);
+    if (errno == EINVAL || endptr == set->values[0])
+        return XLU_CFG_STRING;
+    return XLU_CFG_LONG;
+}
+
 static int find_atom(const XLU_Config *cfg, const char *n,
                      XLU_ConfigSetting **set_r) {
     XLU_ConfigSetting *set;
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index 0262e55..0d7ae91 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -24,6 +24,11 @@
 typedef struct XLU_Config XLU_Config;
 typedef struct XLU_ConfigList XLU_ConfigList;
 
+#define XLU_CFG_NOTFOUND 0
+#define XLU_CFG_LIST     1
+#define XLU_CFG_LONG     2
+#define XLU_CFG_STRING   3
+
 XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename);
   /* 0 means we got ENOMEM. */
   /* report_filename is copied; report is saved and must remain valid
@@ -45,6 +50,7 @@ void xlu_cfg_destroy(XLU_Config*);
  *   ERANGE   value out of range (from strtol)
  */
 
+int xlu_cfg_get_type(const XLU_Config *cfg, const char *n);
 int xlu_cfg_get_string(const XLU_Config*, const char *n, const char **value_r);
 int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r);
 
-- 
1.6.4


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 2/5] libxl: add xlu_cfg_get_type function

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

As cpuid= can be used with two syntaxes (one as a list, the other as a
string), we need to distinguish them without an error message. Introduce
a helper function to detect the type of entry used before issuing a warning.

Signed-off-by: Andre Przywara

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: 0002-libxl-add-xlu_cfg_get_type-function.patch --]
[-- Type: text/x-patch, Size: 2396 bytes --]

>From d7f48fb3caac4b2fc99e9db1a8d82b17bdf4391d Mon Sep 17 00:00:00 2001
From: Andre Przywara <andre.przywara@amd.com>
Date: Wed, 1 Sep 2010 14:11:54 +0200
Subject: [PATCH 2/5] libxl: add xlu_cfg_get_type function

cpuid= can be used with two syntaxes, one as a list, the other as a
string. Introduce a helper function to detect the type of entry used
before issuing a warning.

Signed-off-by: Andre Przywara
---
 tools/libxl/libxlu_cfg.c |   18 ++++++++++++++++++
 tools/libxl/libxlutil.h  |    6 ++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 07e65e1..c19c6ab 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -124,6 +124,24 @@ static XLU_ConfigSetting *find(const XLU_Config *cfg, const char *n) {
     return 0;
 }
 
+int xlu_cfg_get_type(const XLU_Config *cfg, const char *n)
+{
+    XLU_ConfigSetting *set;
+    char *endptr;
+    long l;
+
+    set = find(cfg, n);
+    if (set == NULL)
+        return XLU_CFG_NOTFOUND;
+    if (set->avalues > 1)
+        return XLU_CFG_LIST;
+    errno = 0;
+    l = strtol(set->values[0], &endptr, 0);
+    if (errno == EINVAL || endptr == set->values[0])
+        return XLU_CFG_STRING;
+    return XLU_CFG_LONG;
+}
+
 static int find_atom(const XLU_Config *cfg, const char *n,
                      XLU_ConfigSetting **set_r) {
     XLU_ConfigSetting *set;
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index 6c992a2..adf144e 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -24,6 +24,11 @@
 typedef struct XLU_Config XLU_Config;
 typedef struct XLU_ConfigList XLU_ConfigList;
 
+#define XLU_CFG_NOTFOUND 0
+#define XLU_CFG_LIST     1
+#define XLU_CFG_LONG     2
+#define XLU_CFG_STRING   3
+
 XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename);
   /* 0 means we got ENOMEM. */
   /* report_filename is copied; report is saved and must remain valid
@@ -45,6 +50,7 @@ void xlu_cfg_destroy(XLU_Config*);
  *   ERANGE   value out of range (from strtol)
  */
 
+int xlu_cfg_get_type(const XLU_Config *cfg, const char *n);
 int xlu_cfg_get_string(const XLU_Config*, const char *n, const char **value_r);
 int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n, char **value_r); /* free/strdup version */
 int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r);
-- 
1.6.4


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 2/5] libxl: add xlu_cfg_get_type function

[Ian Jackson]

Andre Przywara writes ("[Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type function"):
> As cpuid= can be used with two syntaxes (one as a list, the other as a
> string), we need to distinguish them without an error message. Introduce
> a helper function to detect the type of entry used before issuing a warning.

Thanks.  This is a generally good idea although I'm not quite
convinced by this:

 +    errno = 0;
 +    l = strtol(set->values[0], &endptr, 0);
 +    if (errno == EINVAL || endptr == set->values[0])
 +        return XLU_CFG_STRING;
 +    return XLU_CFG_LONG;

Firstly, it will fail for unsigned longs bigger than LONG_MAX, and we
would normally think about unsigned longs.  Secondly, if callers say
things like
  if (type == XLU_CFG_STRING) ....
they'll have a bug.

I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
themselves if they need to distinguish numbers from strings.

Ian.

Re: [PATCH 2/5] libxl: add xlu_cfg_get_type function

[Andre Przywara]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

Ian Jackson wrote:
> Andre Przywara writes ("[Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type function"):
>> As cpuid= can be used with two syntaxes (one as a list, the other as a
>> string), we need to distinguish them without an error message. Introduce
>> a helper function to detect the type of entry used before issuing a warning.
> 
> Thanks.  This is a generally good idea although I'm not quite
> convinced by this:
> 
>  +    errno = 0;
>  +    l = strtol(set->values[0], &endptr, 0);
>  +    if (errno == EINVAL || endptr == set->values[0])
>  +        return XLU_CFG_STRING;
>  +    return XLU_CFG_LONG;
> 
> Firstly, it will fail for unsigned longs bigger than LONG_MAX, and we
> would normally think about unsigned longs.
But there is only xlu_cfg_get_long, which returns signed values (used 28 
times in xl_cmdimpl.c). I don't see any usage of strtoul in xl_cmdimpl.c 
which is preceded by xlu_cfg_get_string().

>  Secondly, if callers say things like
>   if (type == XLU_CFG_STRING) ....
> they'll have a bug.
> I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
> themselves if they need to distinguish numbers from strings.
Makes sense. Do you mean like the attached delta patch?

I could also live with making the reporting of the error in 
libxl_cfg_get_list() optional, so that users aren't bothered with a 
confusing error output everytime. That would make the whole function 
obsolete.

Tell me what you like more.


Regards,
Andre.


-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12

[-- Attachment #2: xl_get_atom.patch --]
[-- Type: text/x-patch, Size: 1634 bytes --]

diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index c19c6ab..f9263a6 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -127,19 +127,13 @@ static XLU_ConfigSetting *find(const XLU_Config *cfg, const char *n) {
 int xlu_cfg_get_type(const XLU_Config *cfg, const char *n)
 {
     XLU_ConfigSetting *set;
-    char *endptr;
-    long l;
 
     set = find(cfg, n);
     if (set == NULL)
         return XLU_CFG_NOTFOUND;
     if (set->avalues > 1)
         return XLU_CFG_LIST;
-    errno = 0;
-    l = strtol(set->values[0], &endptr, 0);
-    if (errno == EINVAL || endptr == set->values[0])
-        return XLU_CFG_STRING;
-    return XLU_CFG_LONG;
+    return XLU_CFG_ATOM;
 }
 
 static int find_atom(const XLU_Config *cfg, const char *n,
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index adf144e..e6a75d5 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -26,8 +26,7 @@ typedef struct XLU_ConfigList XLU_ConfigList;
 
 #define XLU_CFG_NOTFOUND 0
 #define XLU_CFG_LIST     1
-#define XLU_CFG_LONG     2
-#define XLU_CFG_STRING   3
+#define XLU_CFG_ATOM     2
 
 XLU_Config *xlu_cfg_init(FILE *report, const char *report_filename);
   /* 0 means we got ENOMEM. */
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ee7f36a..2c90c2b 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1046,7 +1046,7 @@ skip_vfb:
             }
         }
         break;
-    case XLU_CFG_STRING:
+    case XLU_CFG_ATOM:
         if (!xlu_cfg_get_string(config, "cpuid", &buf)) {
             char *buf2, *p, *errstr, *strtok_ptr;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 2/5] libxl: add xlu_cfg_get_type function

[Ian Jackson]

Andre Przywara writes ("Re: [Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type function"):
> But there is only xlu_cfg_get_long, which returns signed values (used 28 
> times in xl_cmdimpl.c). I don't see any usage of strtoul in xl_cmdimpl.c 
> which is preceded by xlu_cfg_get_string().

That's true, although it may not remain so forever.  But my other
arguments stand I think.

> >  Secondly, if callers say things like
> >   if (type == XLU_CFG_STRING) ....
> > they'll have a bug.
> > I would suggest XLU_CFG_ATOM.  Callers can use strto[u]l (or whatever)
> > themselves if they need to distinguish numbers from strings.
>
> Makes sense. Do you mean like the attached delta patch?

Right, yes, that seems sensible.

> I could also live with making the reporting of the error in 
> libxl_cfg_get_list() optional, so that users aren't bothered with a 
> confusing error output everytime. That would make the whole function 
> obsolete.

That would be fine too.

> Tell me what you like more.

My usual rule is "do whatever makes the code smaller".  I guess in
this case that probably means have the error reporting flag on
libxl_cfg_get_list.

Ian.