Re: [Xen-devel] [PATCH v3 18/19] libxlutil: nested list support
Wei Liu writes ("Re: [PATCH v3 18/19] libxlutil: nested list support"):
> On Tue, Jan 13, 2015 at 03:52:48PM +, Ian Jackson wrote:
> > This commit message is very brief. For example, under the heading of
> > `Rework internal representation of setting' I would expect a clear
> > description of every formulaic change.
>
> Originally the internal representation of setting is (string, string)
> pair, the first string being the name of the setting, second string
> being the value of the setting. Now the internal is changed to (string,
> ConfigValue) pair, where ConfigValue can refer to a string or a list of
> ConfigValue's. Internal functions to deal with setting are changed
> accordingly.
>
> Does the above description makes things clearer?
Yes. Something like that should be in the commit message. It would
help to refer to the actual type names. You could say (if true) for
example, "internal functions new refer to a ConfigSetting; the public
APIs still talk about ConfigValues" or some such.
> > Also, I think would be much easier to review if split up into 3 parts,
> > which from the description above ought to be doable without trouble.
>
> OK. I can try to split this patch into three.
If it's difficult for some reason then do get back to me.
> > AFAICT from your changes, the API is not backward compatible. ICBW,
> > but if I'm right that's not acceptable I'm afraid, even in libxlu.
>
> The old APIs still have the same semantic as before, so any applications
> linked against those APIs still have the same results returned.
Oh yes. Sorry, I had misread the patch and read your changes to
libxlu_internal.h as being in libxlutil.h.
> > > Previous APIs work as before.
> >
> > That can't be right because you have to at least specify how they deal
> > with the additional config file syntax.
>
> No, the old APIs don't deal with new syntax. If applications want to
> support new syntax, they need to use new API.
It's obvious that the new API can't return the new syntax. The
question is what happens if you try.
> If old APIs try to get value from new syntax, it has no effect.
I don't think "has no effect" can be right. What is the actual return
value from the function ? Will it be treated as an error ? (IMO it
should be, and this should be documented.)
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 18/19] libxlutil: nested list support
On Tue, Jan 13, 2015 at 03:52:48PM +, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 18/19] libxlutil: nested list support"):
> > This is done with three major changes:
> > 1. Rework internal representation of setting.
> > 2. Extend grammar of parser.
> > 3. Introduce new APIs.
>
> This commit message is very brief. For example, under the heading of
> `Rework internal representation of setting' I would expect a clear
> description of every formulaic change.
>
Originally the internal representation of setting is (string, string)
pair, the first string being the name of the setting, second string
being the value of the setting. Now the internal is changed to (string,
ConfigValue) pair, where ConfigValue can refer to a string or a list of
ConfigValue's. Internal functions to deal with setting are changed
accordingly.
Does the above description makes things clearer?
> Also, I think would be much easier to review if split up into 3 parts,
> which from the description above ought to be doable without trouble.
>
OK. I can try to split this patch into three.
> AFAICT from your changes, the API is not backward compatible. ICBW,
> but if I'm right that's not acceptable I'm afraid, even in libxlu.
>
The old APIs still have the same semantic as before, so any applications
linked against those APIs still have the same results returned.
> > Previous APIs work as before.
>
> That can't be right because you have to at least specify how they deal
> with the additional config file syntax.
>
No, the old APIs don't deal with new syntax. If applications want to
support new syntax, they need to use new API.
If old APIs try to get value from new syntax, it has no effect.
Wei.
> Thanks,
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 18/19] libxlutil: nested list support
Wei Liu writes ("[PATCH v3 18/19] libxlutil: nested list support"):
> This is done with three major changes:
> 1. Rework internal representation of setting.
> 2. Extend grammar of parser.
> 3. Introduce new APIs.
This commit message is very brief. For example, under the heading of
`Rework internal representation of setting' I would expect a clear
description of every formulaic change.
Also, I think would be much easier to review if split up into 3 parts,
which from the description above ought to be doable without trouble.
AFAICT from your changes, the API is not backward compatible. ICBW,
but if I'm right that's not acceptable I'm afraid, even in libxlu.
> Previous APIs work as before.
That can't be right because you have to at least specify how they deal
with the additional config file syntax.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 18/19] libxlutil: nested list support
This is done with three major changes:
1. Rework internal representation of setting.
2. Extend grammar of parser.
3. Introduce new APIs.
New APIs introduced:
1. xlu_cfg_value_type
2. xlu_cfg_value_get_string
3. xlu_cfg_value_get_list
4. xlu_cfg_get_listitem2
Previous APIs work as before.
Nested list will be used to represent two dimensional array used to
specify distances among different vNUMA nodes.
Signed-off-by: Wei Liu
Cc: Ian Jackson
Cc: Ian Campbell
---
tools/libxl/libxlu_cfg.c | 180 +
tools/libxl/libxlu_cfg_i.h| 15 +++-
tools/libxl/libxlu_cfg_y.c| 110 +++--
tools/libxl/libxlu_cfg_y.h|2 +-
tools/libxl/libxlu_cfg_y.y| 19 +++--
tools/libxl/libxlu_internal.h | 24 --
tools/libxl/libxlutil.h | 11 ++-
7 files changed, 208 insertions(+), 153 deletions(-)
diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 22adcb0..db26c34 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -132,13 +132,9 @@ int xlu_cfg_readdata(XLU_Config *cfg, const char *data,
int length) {
}
void xlu__cfg_set_free(XLU_ConfigSetting *set) {
-int i;
-
if (!set) return;
+xlu__cfg_value_free(set->value);
free(set->name);
-for (i=0; invalues; i++)
-free(set->values[i]);
-free(set->values);
free(set);
}
@@ -173,7 +169,7 @@ static int find_atom(const XLU_Config *cfg, const char *n,
set= find(cfg,n);
if (!set) return ESRCH;
-if (set->avalues!=1) {
+if (set->value->type!=XLU_STRING) {
if (!dont_warn)
fprintf(cfg->report,
"%s:%d: warning: parameter `%s' is"
@@ -185,13 +181,30 @@ static int find_atom(const XLU_Config *cfg, const char *n,
return 0;
}
+enum XLU_ConfigValueType xlu_cfg_value_type(const XLU_ConfigValue *value)
+{
+return value->type;
+}
+
+const char *xlu_cfg_value_get_string(const XLU_ConfigValue *value)
+{
+assert(value->type == XLU_STRING);
+return value->u.string;
+}
+
+const XLU_ConfigList *xlu_cfg_value_get_list(const XLU_ConfigValue *value)
+{
+assert(value->type == XLU_LIST);
+return &value->u.list;
+}
+
int xlu_cfg_get_string(const XLU_Config *cfg, const char *n,
const char **value_r, int dont_warn) {
XLU_ConfigSetting *set;
int e;
e= find_atom(cfg,n,&set,dont_warn); if (e) return e;
-*value_r= set->values[0];
+*value_r= set->value->u.string;
return 0;
}
@@ -202,7 +215,7 @@ int xlu_cfg_replace_string(const XLU_Config *cfg, const
char *n,
e= find_atom(cfg,n,&set,dont_warn); if (e) return e;
free(*value_r);
-*value_r= strdup(set->values[0]);
+*value_r= strdup(set->value->u.string);
return 0;
}
@@ -214,7 +227,7 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
char *ep;
e= find_atom(cfg,n,&set,dont_warn); if (e) return e;
-errno= 0; l= strtol(set->values[0], &ep, 0);
+errno= 0; l= strtol(set->value->u.string, &ep, 0);
e= errno;
if (errno) {
e= errno;
@@ -226,7 +239,7 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const char *n,
cfg->config_source, set->lineno, n, strerror(e));
return e;
}
-if (*ep || ep==set->values[0]) {
+if (*ep || ep==set->value->u.string) {
if (!dont_warn)
fprintf(cfg->report,
"%s:%d: warning: parameter `%s' is not a valid number\n",
@@ -253,7 +266,7 @@ int xlu_cfg_get_list(const XLU_Config *cfg, const char *n,
XLU_ConfigList **list_r, int *entries_r, int dont_warn) {
XLU_ConfigSetting *set;
set= find(cfg,n); if (!set) return ESRCH;
-if (set->avalues==1) {
+if (set->value->type!=XLU_LIST) {
if (!dont_warn) {
fprintf(cfg->report,
"%s:%d: warning: parameter `%s' is a single value"
@@ -262,8 +275,8 @@ int xlu_cfg_get_list(const XLU_Config *cfg, const char *n,
}
return EINVAL;
}
-if (list_r) *list_r= set;
-if (entries_r) *entries_r= set->nvalues;
+if (list_r) *list_r= &set->value->u.list;
+if (entries_r) *entries_r= set->value->u.list.nvalues;
return 0;
}
@@ -290,72 +303,29 @@ int xlu_cfg_get_list_as_string_list(const XLU_Config
*cfg, const char *n,
return 0;
}
-const char *xlu_cfg_get_listitem(const XLU_ConfigList *set, int entry) {
-if (entry < 0 || entry >= set->nvalues) return 0;
-return set->values[entry];
+const char *xlu_cfg_get_listitem(const XLU_ConfigList *list, int entry) {
+if (entry < 0 || entry >= list->nvalues) return 0;
+if (list->values[entry]->type != XLU_STRING) return 0;
+return list->values[entry]->u.string;
}
-
-XLU_ConfigSetting *xlu__cfg_set_mk(CfgParseContext *ctx,
- int alloc, char *atom) {
-XLU_ConfigSetting *set= 0;
-
-if (ctx->err) goto x;
-assert(!!alloc == !!atom
