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
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;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home