This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type function
From: Andre Przywara <andre.przywara@xxxxxxx>
Date: Fri, 17 Sep 2010 13:38:51 +0200
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Keir Fraser <keir.fraser@xxxxxxxxxxxxx>, Ian Campbell <Ian.Campbell@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Fri, 17 Sep 2010 04:44:41 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <19602.20530.392609.592745@xxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C921675.2030702@xxxxxxx> <19602.20530.392609.592745@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird (X11/20090820)
Ian Jackson wrote:
Andre Przywara writes ("[Xen-devel] [PATCH 2/5] libxl: add xlu_cfg_get_type 
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.


Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 448-3567-12
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_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:
-    case XLU_CFG_STRING:
+    case XLU_CFG_ATOM:
         if (!xlu_cfg_get_string(config, "cpuid", &buf)) {
             char *buf2, *p, *errstr, *strtok_ptr;
Xen-devel mailing list