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

[Andre Przywara <andre.przywara@amd.com>]

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