From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 2/5] libxl: add xlu_cfg_get_type function Date: Fri, 17 Sep 2010 13:38:51 +0200 Message-ID: <4C93534B.60008@amd.com> References: <4C921675.2030702@amd.com> <19602.20530.392609.592745@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070206030300060807070002" Return-path: In-Reply-To: <19602.20530.392609.592745@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Jackson Cc: xen-devel , Keir Fraser , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org --------------070206030300060807070002 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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 --------------070206030300060807070002 Content-Type: text/x-patch; name="xl_get_atom.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="xl_get_atom.patch" 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; --------------070206030300060807070002 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------070206030300060807070002--