| Message ID | 1515408128-31361-1-git-send-email-stefan@herbrechtsmeier.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | parser: Replace properties array with a group of properties | expand |
Hi Stefan,
I am testing this (I have not checked deeply the code). I want just
check the behavior.
It looks like that it is not robust enogh. In fact, running with an
older sw-description, I get a segfault:
[TRACE] : SWUPDATE running : [add_properties_cb] : Property (null):
(null)
Segmentation fault (core dumped)
This is a sw-description with array as:
properties: (
{
name = "url";
value = "http://192.168.178.41:8080";
},
{
name = "url";
value = "http://192.168.178.41:8080";
}
);
With the following sw-description, it works:
properties: {
url = "http://192.168.178.41:8080";
url1 = "http://192.168.178.41:8080";
};
But enclosing in parenthesis, it crashes with segfault,too:
properties: ({
url = "http://192.168.178.41:8080";
url1 = "http://192.168.178.41:8080";
});
It is ok if it does not find anything, but of course it is not ok if
crashes. sw-description can be completely wrong, and SWUpdate must just
return with an error.
Nevertheless, I have problem (see above) when I need multiple instances
of the same property. I change above "url" to "url1" to test with your
code, but the original sw-description is :
properties: (
{
name = "url";
value = "http://192.168.178.41:8080";
},
{
name = "url";
value = "http://192.168.178.42:8080";
}
);
Note: this is a real requirement. The SWUforward handler collects all
urls and send an artifact (again a SWU) to remote targets running SWUpdate.
Moving away from properties group, I am expecting to write something like:
properties: ({
url = ["http://192.168.178.41:8080",
"http://192.168.178.42:8080"];
});
(it crashes, too).
I have briefly tested JSON, too, but I get the same issues.
Best regards,
Stefano
On 08/01/2018 11:42, stefan@herbrechtsmeier.net wrote:
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
> corelib/parsing_library.c | 15 +++++++++++++++
> corelib/parsing_library_libconfig.c | 15 +++++++++++++++
> corelib/parsing_library_libjson.c | 12 ++++++++++++
> include/parselib.h | 9 +++++++++
> parser/parser.c | 32 ++++++++++++--------------------
> 5 files changed, 63 insertions(+), 20 deletions(-)
>
> diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c
> index 79f3886..8f4637d 100644
> --- a/corelib/parsing_library.c
> +++ b/corelib/parsing_library.c
> @@ -74,6 +74,21 @@ void *get_child(parsertype p, void *e, const char *name)
> return NULL;
> }
>
> +void iterate_field(parsertype p, void *e, iterate_callback cb, void *data)
> +{
> + switch (p) {
> + case LIBCFG_PARSER:
> + iterate_field_libconfig(e, cb, data);
> + break;
> + case JSON_PARSER:
> + iterate_field_json(e, cb, data);
> + break;
> + default:
> + (void)e;
> + (void)data;
> + }
> +}
> +
> void *get_elem_from_idx(parsertype p, void *node, int idx)
> {
> switch (p) {
> diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c
> index f0cfb38..30e0822 100644
> --- a/corelib/parsing_library_libconfig.c
> +++ b/corelib/parsing_library_libconfig.c
> @@ -59,6 +59,21 @@ void *get_child_libconfig(void *e, const char *name)
> return config_setting_get_member(e, name);
> }
>
> +void iterate_field_libconfig(config_setting_t *e, iterate_callback cb, void *data)
> +{
> + config_setting_t *elem;
> + const char *str;
> + int i;
> +
> + for (i = 0; i < config_setting_length(e); i++) {
> + elem = config_setting_get_elem(e, i);
> + str = config_setting_get_string(elem);
> +
> + if (cb)
> + cb(elem->name, str, data);
> + }
> +}
> +
> void get_field_cfg(config_setting_t *e, const char *path, void *dest)
> {
> config_setting_t *elem;
> diff --git a/corelib/parsing_library_libjson.c b/corelib/parsing_library_libjson.c
> index 67cbc2c..c6a3a24 100644
> --- a/corelib/parsing_library_libjson.c
> +++ b/corelib/parsing_library_libjson.c
> @@ -63,6 +63,18 @@ void *get_child_json(json_object *e, const char *path)
> return node;
> }
>
> +void iterate_field_json(json_object *e, iterate_callback cb, void *data)
> +{
> + const char *str;
> +
> + json_object_object_foreach(e, key, node) {
> + str = json_object_get_string(node);
> +
> + if (cb)
> + cb(key, str, data);
> + }
> +}
> +
> const char *get_field_string_json(json_object *e, const char *path)
> {
> const char *str;
> diff --git a/include/parselib.h b/include/parselib.h
> index 7c44a5e..b63349c 100644
> --- a/include/parselib.h
> +++ b/include/parselib.h
> @@ -27,6 +27,9 @@ typedef enum {
> JSON_PARSER
> } parsertype;
>
> +typedef void (*iterate_callback)(const char *name, const char *value,
> + void *data);
> +
> #ifdef CONFIG_LIBCONFIG
> #include <libconfig.h>
> #define LIBCONFIG_VERSION ((LIBCONFIG_VER_MAJOR << 16) | \
> @@ -38,6 +41,8 @@ typedef enum {
> void get_value_libconfig(const config_setting_t *e, void *dest);
> void get_field_cfg(config_setting_t *e, const char *path, void *dest);
> void *get_child_libconfig(void *e, const char *name);
> +void iterate_field_libconfig(config_setting_t *e, iterate_callback cb,
> + void *data);
> const char *get_field_string_libconfig(config_setting_t *e, const char *path);
>
> #else
> @@ -47,6 +52,7 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path);
> #define find_node_libconfig(cfg, field, swcfg) (NULL)
> #define get_field_string_libconfig(e, path) (NULL)
> #define get_child_libconfig(e, name) (NULL)
> +#define iterate_field_libconfig(e, cb, data) (0)
> #define get_field_cfg(e, path, dest)
> #endif
>
> @@ -57,6 +63,7 @@ const char *get_field_string_json(json_object *e, const char *path);
> void get_value_json(json_object *e, void *dest);
> void get_field_json(json_object *e, const char *path, void *dest);
> void *get_child_json(json_object *e, const char *name);
> +void iterate_field_json(json_object *e, iterate_callback cb, void *data);
> json_object *find_json_recursive_node(json_object *root, const char **names);
> json_object *json_get_key(json_object *json_root, const char *key);
> const char *json_get_value(struct json_object *json_root,
> @@ -68,6 +75,7 @@ char *json_get_data_url(json_object *json_root, const char *key);
> #define find_node_json(a, b, c) (NULL)
> #define get_field_string_json(e, path) (NULL)
> #define get_child_json(e, name) (NULL)
> +#define iterate_field_json(e, cb, data) (0)
> #define get_field_json(e, path, dest)
> #define json_object_object_get_ex(a,b,c) (0)
> #define json_object_array_get_idx(a, b) (0)
> @@ -82,6 +90,7 @@ void get_field_string_with_size(parsertype p, void *e, const char *path,
> int get_array_length(parsertype p, void *root);
> void *get_elem_from_idx(parsertype p, void *node, int idx);
> void *get_child(parsertype p, void *node, const char *name);
> +void iterate_field(parsertype p, void *e, iterate_callback cb, void *data);
> void get_field(parsertype p, void *e, const char *path, void *dest);
> int exist_field_string(parsertype p, void *e, const char *path);
> void get_hash_value(parsertype p, void *elem, unsigned char *hash);
> diff --git a/parser/parser.c b/parser/parser.c
> index 44cd0fe..d4fcb11 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -152,32 +152,24 @@ static void *find_node(parsertype p, void *root, const char *node,
> return NULL;
> }
>
> -static void add_properties(parsertype p, void *node, struct img_type *image)
> +static void add_properties_cb(const char *name, const char *value, void *data)
> {
> + struct img_type *image = (struct img_type *)data;
>
> - void *properties, *prop;
> - int count, i;
> + TRACE("\t\tProperty %s: %s", name, value);
> + if (dict_insert_entry(&image->properties, (char *)name, (char *)value))
> + ERROR("Property not stored, skipping...");
> +}
> +
> +static void add_properties(parsertype p, void *node, struct img_type *image)
> +{
> + void *properties;
>
> properties = get_child(p, node, "properties");
> if (properties) {
> - count = get_array_length(p, properties);
> -
> - TRACE("Found %d properties for %s:", count, image->fname);
> -
> - for (i = 0; i < count; i++) {
> - char key[255];
> - char value[255];
> - prop = get_elem_from_idx(p, properties, i);
> - GET_FIELD_STRING(p, prop, "name", key);
> - GET_FIELD_STRING(p, prop, "value", value);
> - TRACE("\t\tProperty %d: name=%s val=%s ", i,
> - key,
> - value
> - );
> - if (dict_insert_entry(&image->properties, key, value))
> - ERROR("Property not stored, skipping...");
> + TRACE("Found properties for %s:", image->fname);
>
> - }
> + iterate_field(p, properties, add_properties_cb, image);
> }
> }
>
>
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Mittwoch, 10. Januar 2018 16:26 > An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Cc: Herbrechtsmeier Dr.-Ing. , Stefan > <Stefan.Herbrechtsmeier@weidmueller.com> > Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a > group of properties > > I am testing this (I have not checked deeply the code). I want just check the > behavior. > > It looks like that it is not robust enogh. In fact, running with an older sw- > description, I get a segfault: > > > [TRACE] : SWUPDATE running : [add_properties_cb] : > Property (null): > (null) > Segmentation fault (core dumped) > > This is a sw-description with array as: > > properties: ( > { > name = "url"; > value = "http://192.168.178.41:8080"; > }, > { > name = "url"; > value = "http://192.168.178.41:8080"; > } > ); > > With the following sw-description, it works: > > properties: { > url = "http://192.168.178.41:8080"; > url1 = "http://192.168.178.41:8080"; }; > > But enclosing in parenthesis, it crashes with segfault,too: > > properties: ({ > url = "http://192.168.178.41:8080"; > url1 = "http://192.168.178.41:8080"; }); > > It is ok if it does not find anything, but of course it is not ok if crashes. sw- > description can be completely wrong, and SWUpdate must just return with > an error. The main problem is a missing test of str != NULL in iterate_field_* or add_properties_cb functions. > Nevertheless, I have problem (see above) when I need multiple instances of > the same property. I change above "url" to "url1" to test with your code, but > the original sw-description is : > > properties: ( > { > name = "url"; > value = "http://192.168.178.41:8080"; > }, > { > name = "url"; > value = "http://192.168.178.42:8080"; > } > ); > You couldn't use the same key multiple times because this isn't supported by libconfig and json. > Note: this is a real requirement. The SWUforward handler collects all urls and > send an artifact (again a SWU) to remote targets running SWUpdate. > > Moving away from properties group, I am expecting to write something like: > > properties: ({ > url = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; > }); > > (it crashes, too). The problem is that the software use a dictionary to save the values and this represents a key value pair. You recently changed the implementation to react like a list. I think we should distinguish between the two concepts. I see two solutions to support multiple values inside properties: a) Support only strings. The implementation uses a single string with a coma separated list of values: properties: { urls = "http://192.168.178.41:8080, http://192.168.178.42:8080"; }; b) Support strings and arrays. The implementation saves every property in a link list inside a dictionary: properties: { urls = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; }; The first solution should work out of the box. Best regards Stefan Herbrechtsmeier Software Developer Embedded Systems Weidmüller – Your partner in Industrial Connectivity We look forward to sharing ideas with you – Let’s connect. Weidmueller Interface GmbH & Co. KG Klingenbergstraße 16, 32758 Detmold, Germany Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
Hi Stefan, On 10/01/2018 17:39, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Mittwoch, 10. Januar 2018 16:26 >> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >> <Stefan.Herbrechtsmeier@weidmueller.com> >> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a >> group of properties >> >> I am testing this (I have not checked deeply the code). I want just check the >> behavior. >> >> It looks like that it is not robust enogh. In fact, running with an older sw- >> description, I get a segfault: >> >> >> [TRACE] : SWUPDATE running : [add_properties_cb] : >> Property (null): >> (null) >> Segmentation fault (core dumped) >> >> This is a sw-description with array as: >> >> properties: ( >> { >> name = "url"; >> value = "http://192.168.178.41:8080"; >> }, >> { >> name = "url"; >> value = "http://192.168.178.41:8080"; >> } >> ); >> >> With the following sw-description, it works: >> >> properties: { >> url = "http://192.168.178.41:8080"; >> url1 = "http://192.168.178.41:8080"; }; >> >> But enclosing in parenthesis, it crashes with segfault,too: >> >> properties: ({ >> url = "http://192.168.178.41:8080"; >> url1 = "http://192.168.178.41:8080"; }); >> >> It is ok if it does not find anything, but of course it is not ok if crashes. sw- >> description can be completely wrong, and SWUpdate must just return with >> an error. > > The main problem is a missing test of str != NULL in iterate_field_* or add_properties_cb functions. ok > >> Nevertheless, I have problem (see above) when I need multiple instances of >> the same property. I change above "url" to "url1" to test with your code, but >> the original sw-description is : >> >> properties: ( >> { >> name = "url"; >> value = "http://192.168.178.41:8080"; >> }, >> { >> name = "url"; >> value = "http://192.168.178.42:8080"; >> } >> ); >> > > You couldn't use the same key multiple times because this isn't supported by libconfig and json. I know - in fact, there is up now an array. But it is not the best solution, as you have already let me note. > >> Note: this is a real requirement. The SWUforward handler collects all urls and >> send an artifact (again a SWU) to remote targets running SWUpdate. >> >> Moving away from properties group, I am expecting to write something like: >> >> properties: ({ >> url = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; >> }); >> >> (it crashes, too). > > The problem is that the software use a dictionary to save the values and this represents a key value pair. You recently changed the implementation to react like a list. Exactly. >I think we should distinguish between the two concepts. > > I see two solutions to support multiple values inside properties: > a) Support only strings. The implementation uses a single string with a coma separated list of values: > properties: { > urls = "http://192.168.178.41:8080, http://192.168.178.42:8080"; > }; I have started with this, and this is still present. There is since a long time a generic "data" attribute, that can have a set of values separated by a ":". However, I wanted to go away from this solution because it does not scale well and it becomes difficult to read. We have a language (libconfig or json), and we should use it. > b) Support strings and arrays. The implementation saves every property in a link list inside a dictionary: > properties: { > urls = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; > }; See above, this is the preferred solution for a SWUpdate users. Syntax is clean and straightforward. > > The first solution should work out of the box. I know, but it is nasty. And it is not general enough, because each user of the property (handler, ...) must reparsed again the string. It has also the drawback that the handler runs at a different time as the parser and a syntatical error in the string (for example, missing ",") will be shown later and not at parsing time. So a) is easy but nasty, b) is a better solution. Best regards, Stefano Babic
Hi Stefano, Am 10.01.2018 um 17:53 schrieb Stefano Babic: > On 10/01/2018 17:39, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>> -----Ursprüngliche Nachricht----- >>> Von: Stefano Babic [mailto:sbabic@denx.de] >>> Gesendet: Mittwoch, 10. Januar 2018 16:26 >>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >>> <Stefan.Herbrechtsmeier@weidmueller.com> >>> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a >>> group of properties >>> >>> I am testing this (I have not checked deeply the code). I want just check the >>> behavior. >>> >>> It looks like that it is not robust enogh. In fact, running with an older sw- >>> description, I get a segfault: >>> >>> >>> [TRACE] : SWUPDATE running : [add_properties_cb] : >>> Property (null): >>> (null) >>> Segmentation fault (core dumped) >>> >>> This is a sw-description with array as: >>> >>> properties: ( >>> { >>> name = "url"; >>> value = "http://192.168.178.41:8080"; >>> }, >>> { >>> name = "url"; >>> value = "http://192.168.178.41:8080"; >>> } >>> ); >>> >>> With the following sw-description, it works: >>> >>> properties: { >>> url = "http://192.168.178.41:8080"; >>> url1 = "http://192.168.178.41:8080"; }; >>> >>> But enclosing in parenthesis, it crashes with segfault,too: >>> >>> properties: ({ >>> url = "http://192.168.178.41:8080"; >>> url1 = "http://192.168.178.41:8080"; }); >>> >>> It is ok if it does not find anything, but of course it is not ok if crashes. sw- >>> description can be completely wrong, and SWUpdate must just return with >>> an error. >> The main problem is a missing test of str != NULL in iterate_field_* or add_properties_cb functions. > ok Should I resend the patch with a fix or direct rework the patch to support arrays? >>> Nevertheless, I have problem (see above) when I need multiple instances of >>> the same property. I change above "url" to "url1" to test with your code, but >>> the original sw-description is : >>> >>> properties: ( >>> { >>> name = "url"; >>> value = "http://192.168.178.41:8080"; >>> }, >>> { >>> name = "url"; >>> value = "http://192.168.178.42:8080"; >>> } >>> ); >>> >> You couldn't use the same key multiple times because this isn't supported by libconfig and json. > I know - in fact, there is up now an array. But it is not the best > solution, as you have already let me note. > >>> Note: this is a real requirement. The SWUforward handler collects all urls and >>> send an artifact (again a SWU) to remote targets running SWUpdate. >>> >>> Moving away from properties group, I am expecting to write something like: >>> >>> properties: ({ >>> url = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; >>> }); >>> >>> (it crashes, too). >> The problem is that the software use a dictionary to save the values and this represents a key value pair. You recently changed the implementation to react like a list. > Exactly. > >> I think we should distinguish between the two concepts. >> >> I see two solutions to support multiple values inside properties: >> a) Support only strings. The implementation uses a single string with a coma separated list of values: >> properties: { >> urls = "http://192.168.178.41:8080, http://192.168.178.42:8080"; >> }; > I have started with this, and this is still present. There is since a > long time a generic "data" attribute, that can have a set of values > separated by a ":". > > However, I wanted to go away from this solution because it does not > scale well and it becomes difficult to read. We have a language > (libconfig or json), and we should use it. > >> b) Support strings and arrays. The implementation saves every property in a link list inside a dictionary: >> properties: { >> urls = ["http://192.168.178.41:8080", "http://192.168.178.42:8080"]; >> }; > See above, this is the preferred solution for a SWUpdate users. Syntax > is clean and straightforward. > >> The first solution should work out of the box. > I know, but it is nasty. And it is not general enough, because each user > of the property (handler, ...) must reparsed again the string. It has > also the drawback that the handler runs at a different time as the > parser and a syntatical error in the string (for example, missing ",") > will be shown later and not at parsing time. > > So a) is easy but nasty, b) is a better solution. Any preferences how b) shout be implemented? Best regards Stefan
Hi Stefan, On 10/01/2018 19:47, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > > Am 10.01.2018 um 17:53 schrieb Stefano Babic: >> On 10/01/2018 17:39, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>> -----Ursprüngliche Nachricht----- >>>> Von: Stefano Babic [mailto:sbabic@denx.de] >>>> Gesendet: Mittwoch, 10. Januar 2018 16:26 >>>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >>>> <Stefan.Herbrechtsmeier@weidmueller.com> >>>> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a >>>> group of properties >>>> >>>> I am testing this (I have not checked deeply the code). I want just >>>> check the >>>> behavior. >>>> >>>> It looks like that it is not robust enogh. In fact, running with an >>>> older sw- >>>> description, I get a segfault: >>>> >>>> >>>> [TRACE] : SWUPDATE running : [add_properties_cb] : >>>> Property (null): >>>> (null) >>>> Segmentation fault (core dumped) >>>> >>>> This is a sw-description with array as: >>>> >>>> properties: ( >>>> { >>>> name = "url"; >>>> value = "http://192.168.178.41:8080"; >>>> }, >>>> { >>>> name = "url"; >>>> value = "http://192.168.178.41:8080"; >>>> } >>>> ); >>>> >>>> With the following sw-description, it works: >>>> >>>> properties: { >>>> url = "http://192.168.178.41:8080"; >>>> url1 = "http://192.168.178.41:8080"; }; >>>> >>>> But enclosing in parenthesis, it crashes with segfault,too: >>>> >>>> properties: ({ >>>> url = "http://192.168.178.41:8080"; >>>> url1 = "http://192.168.178.41:8080"; }); >>>> >>>> It is ok if it does not find anything, but of course it is not ok if >>>> crashes. sw- >>>> description can be completely wrong, and SWUpdate must just return with >>>> an error. >>> The main problem is a missing test of str != NULL in iterate_field_* >>> or add_properties_cb functions. >> ok > > Should I resend the patch with a fix or direct rework the patch to > support arrays? No, please rework to support arrays, too, thanks ! > >>>> Nevertheless, I have problem (see above) when I need multiple >>>> instances of >>>> the same property. I change above "url" to "url1" to test with your >>>> code, but >>>> the original sw-description is : >>>> >>>> properties: ( >>>> { >>>> name = "url"; >>>> value = "http://192.168.178.41:8080"; >>>> }, >>>> { >>>> name = "url"; >>>> value = "http://192.168.178.42:8080"; >>>> } >>>> ); >>>> >>> You couldn't use the same key multiple times because this isn't >>> supported by libconfig and json. >> I know - in fact, there is up now an array. But it is not the best >> solution, as you have already let me note. >> >>>> Note: this is a real requirement. The SWUforward handler collects >>>> all urls and >>>> send an artifact (again a SWU) to remote targets running SWUpdate. >>>> >>>> Moving away from properties group, I am expecting to write something >>>> like: >>>> >>>> properties: ({ >>>> url = ["http://192.168.178.41:8080", >>>> "http://192.168.178.42:8080"]; >>>> }); >>>> >>>> (it crashes, too). >>> The problem is that the software use a dictionary to save the values >>> and this represents a key value pair. You recently changed the >>> implementation to react like a list. >> Exactly. >> >>> I think we should distinguish between the two concepts. >>> >>> I see two solutions to support multiple values inside properties: >>> a) Support only strings. The implementation uses a single string with >>> a coma separated list of values: >>> properties: { >>> urls = "http://192.168.178.41:8080, >>> http://192.168.178.42:8080"; >>> }; >> I have started with this, and this is still present. There is since a >> long time a generic "data" attribute, that can have a set of values >> separated by a ":". >> >> However, I wanted to go away from this solution because it does not >> scale well and it becomes difficult to read. We have a language >> (libconfig or json), and we should use it. >> >>> b) Support strings and arrays. The implementation saves every >>> property in a link list inside a dictionary: >>> properties: { >>> urls = ["http://192.168.178.41:8080", >>> "http://192.168.178.42:8080"]; >>> }; >> See above, this is the preferred solution for a SWUpdate users. Syntax >> is clean and straightforward. >> >>> The first solution should work out of the box. >> I know, but it is nasty. And it is not general enough, because each user >> of the property (handler, ...) must reparsed again the string. It has >> also the drawback that the handler runs at a different time as the >> parser and a syntatical error in the string (for example, missing ",") >> will be shown later and not at parsing time. >> >> So a) is easy but nasty, b) is a better solution. > Any preferences how b) shout be implemented? I do not think we need a more complex structure to stor eproperties, so a linked-list with pairs name / value as we have already should be sufficient. Or what do you mind ? Best regards, Stefano Babic
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Donnerstag, 11. Januar 2018 09:29 > An: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>; Stefano Babic > <sbabic@denx.de>; Herbrechtsmeier Dr.-Ing. , Stefan > <Stefan.Herbrechtsmeier@weidmueller.com>; > swupdate@googlegroups.com > Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a > group of properties > > On 10/01/2018 19:47, Stefan Herbrechtsmeier wrote: > > > > Am 10.01.2018 um 17:53 schrieb Stefano Babic: > >> On 10/01/2018 17:39, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >>>> -----Ursprüngliche Nachricht----- > >>>> Von: Stefano Babic [mailto:sbabic@denx.de] > >>>> Gesendet: Mittwoch, 10. Januar 2018 16:26 > >>>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan > >>>> <Stefan.Herbrechtsmeier@weidmueller.com> > >>>> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array > >>>> with a group of properties > >>>> <snip> > >>> I see two solutions to support multiple values inside properties: > >>> a) Support only strings. The implementation uses a single string > >>> with a coma separated list of values: > >>> properties: { > >>> urls = "http://192.168.178.41:8080, > >>> http://192.168.178.42:8080"; > >>> }; > >> I have started with this, and this is still present. There is since a > >> long time a generic "data" attribute, that can have a set of values > >> separated by a ":". > >> > >> However, I wanted to go away from this solution because it does not > >> scale well and it becomes difficult to read. We have a language > >> (libconfig or json), and we should use it. > >> > >>> b) Support strings and arrays. The implementation saves every > >>> property in a link list inside a dictionary: > >>> properties: { > >>> urls = ["http://192.168.178.41:8080", > >>> "http://192.168.178.42:8080"]; > >>> }; > >> See above, this is the preferred solution for a SWUpdate users. > >> Syntax is clean and straightforward. > >> > >>> The first solution should work out of the box. > >> I know, but it is nasty. And it is not general enough, because each > >> user of the property (handler, ...) must reparsed again the string. > >> It has also the drawback that the handler runs at a different time as > >> the parser and a syntatical error in the string (for example, missing > >> ",") will be shown later and not at parsing time. > >> > >> So a) is easy but nasty, b) is a better solution. > > Any preferences how b) shout be implemented? > > I do not think we need a more complex structure to stor eproperties, so a > linked-list with pairs name / value as we have already should be sufficient. Or > what do you mind ? I think of a property dict with a value list to keep the structure from the config files. Therefore I would rework the dict_get_value function to return the first entry from the value list and the dict_insert_entry function to insert an entry in the value list of the key. An additional dict_get_list return the value list to iterate over all entries of a key. The Lua property table will use a string if the list has one entry and a table otherwise. Best regards Stefan Herbrechtsmeier Software Developer Embedded Systems Weidmüller – Your partner in Industrial Connectivity We look forward to sharing ideas with you – Let’s connect. Weidmueller Interface GmbH & Co. KG Klingenbergstraße 16, 32758 Detmold, Germany Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
On 11/01/2018 09:50, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Donnerstag, 11. Januar 2018 09:29 >> An: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>; Stefano Babic >> <sbabic@denx.de>; Herbrechtsmeier Dr.-Ing. , Stefan >> <Stefan.Herbrechtsmeier@weidmueller.com>; >> swupdate@googlegroups.com >> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array with a >> group of properties >> >> On 10/01/2018 19:47, Stefan Herbrechtsmeier wrote: >>> >>> Am 10.01.2018 um 17:53 schrieb Stefano Babic: >>>> On 10/01/2018 17:39, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>>>>> -----Ursprüngliche Nachricht----- >>>>>> Von: Stefano Babic [mailto:sbabic@denx.de] >>>>>> Gesendet: Mittwoch, 10. Januar 2018 16:26 >>>>>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >>>>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >>>>>> <Stefan.Herbrechtsmeier@weidmueller.com> >>>>>> Betreff: Re: [swupdate] [PATCH] parser: Replace properties array >>>>>> with a group of properties >>>>>> > > <snip> > >>>>> I see two solutions to support multiple values inside properties: >>>>> a) Support only strings. The implementation uses a single string >>>>> with a coma separated list of values: >>>>> properties: { >>>>> urls = "http://192.168.178.41:8080, >>>>> http://192.168.178.42:8080"; >>>>> }; >>>> I have started with this, and this is still present. There is since a >>>> long time a generic "data" attribute, that can have a set of values >>>> separated by a ":". >>>> >>>> However, I wanted to go away from this solution because it does not >>>> scale well and it becomes difficult to read. We have a language >>>> (libconfig or json), and we should use it. >>>> >>>>> b) Support strings and arrays. The implementation saves every >>>>> property in a link list inside a dictionary: >>>>> properties: { >>>>> urls = ["http://192.168.178.41:8080", >>>>> "http://192.168.178.42:8080"]; >>>>> }; >>>> See above, this is the preferred solution for a SWUpdate users. >>>> Syntax is clean and straightforward. >>>> >>>>> The first solution should work out of the box. >>>> I know, but it is nasty. And it is not general enough, because each >>>> user of the property (handler, ...) must reparsed again the string. >>>> It has also the drawback that the handler runs at a different time as >>>> the parser and a syntatical error in the string (for example, missing >>>> ",") will be shown later and not at parsing time. >>>> >>>> So a) is easy but nasty, b) is a better solution. >>> Any preferences how b) shout be implemented? >> >> I do not think we need a more complex structure to stor eproperties, so a >> linked-list with pairs name / value as we have already should be sufficient. Or >> what do you mind ? > > I think of a property dict with a value list to keep the structure from the config files. Therefore I would rework the dict_get_value function to return the first entry from the value list and the dict_insert_entry function to insert an entry in the value list of the key. An additional dict_get_list return the value list to iterate over all entries of a key. The Lua property table will use a string if the list has one entry and a table otherwise. Fine with me. Best regards, Stefano
diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c index 79f3886..8f4637d 100644 --- a/corelib/parsing_library.c +++ b/corelib/parsing_library.c @@ -74,6 +74,21 @@ void *get_child(parsertype p, void *e, const char *name) return NULL; } +void iterate_field(parsertype p, void *e, iterate_callback cb, void *data) +{ + switch (p) { + case LIBCFG_PARSER: + iterate_field_libconfig(e, cb, data); + break; + case JSON_PARSER: + iterate_field_json(e, cb, data); + break; + default: + (void)e; + (void)data; + } +} + void *get_elem_from_idx(parsertype p, void *node, int idx) { switch (p) { diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c index f0cfb38..30e0822 100644 --- a/corelib/parsing_library_libconfig.c +++ b/corelib/parsing_library_libconfig.c @@ -59,6 +59,21 @@ void *get_child_libconfig(void *e, const char *name) return config_setting_get_member(e, name); } +void iterate_field_libconfig(config_setting_t *e, iterate_callback cb, void *data) +{ + config_setting_t *elem; + const char *str; + int i; + + for (i = 0; i < config_setting_length(e); i++) { + elem = config_setting_get_elem(e, i); + str = config_setting_get_string(elem); + + if (cb) + cb(elem->name, str, data); + } +} + void get_field_cfg(config_setting_t *e, const char *path, void *dest) { config_setting_t *elem; diff --git a/corelib/parsing_library_libjson.c b/corelib/parsing_library_libjson.c index 67cbc2c..c6a3a24 100644 --- a/corelib/parsing_library_libjson.c +++ b/corelib/parsing_library_libjson.c @@ -63,6 +63,18 @@ void *get_child_json(json_object *e, const char *path) return node; } +void iterate_field_json(json_object *e, iterate_callback cb, void *data) +{ + const char *str; + + json_object_object_foreach(e, key, node) { + str = json_object_get_string(node); + + if (cb) + cb(key, str, data); + } +} + const char *get_field_string_json(json_object *e, const char *path) { const char *str; diff --git a/include/parselib.h b/include/parselib.h index 7c44a5e..b63349c 100644 --- a/include/parselib.h +++ b/include/parselib.h @@ -27,6 +27,9 @@ typedef enum { JSON_PARSER } parsertype; +typedef void (*iterate_callback)(const char *name, const char *value, + void *data); + #ifdef CONFIG_LIBCONFIG #include <libconfig.h> #define LIBCONFIG_VERSION ((LIBCONFIG_VER_MAJOR << 16) | \ @@ -38,6 +41,8 @@ typedef enum { void get_value_libconfig(const config_setting_t *e, void *dest); void get_field_cfg(config_setting_t *e, const char *path, void *dest); void *get_child_libconfig(void *e, const char *name); +void iterate_field_libconfig(config_setting_t *e, iterate_callback cb, + void *data); const char *get_field_string_libconfig(config_setting_t *e, const char *path); #else @@ -47,6 +52,7 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path); #define find_node_libconfig(cfg, field, swcfg) (NULL) #define get_field_string_libconfig(e, path) (NULL) #define get_child_libconfig(e, name) (NULL) +#define iterate_field_libconfig(e, cb, data) (0) #define get_field_cfg(e, path, dest) #endif @@ -57,6 +63,7 @@ const char *get_field_string_json(json_object *e, const char *path); void get_value_json(json_object *e, void *dest); void get_field_json(json_object *e, const char *path, void *dest); void *get_child_json(json_object *e, const char *name); +void iterate_field_json(json_object *e, iterate_callback cb, void *data); json_object *find_json_recursive_node(json_object *root, const char **names); json_object *json_get_key(json_object *json_root, const char *key); const char *json_get_value(struct json_object *json_root, @@ -68,6 +75,7 @@ char *json_get_data_url(json_object *json_root, const char *key); #define find_node_json(a, b, c) (NULL) #define get_field_string_json(e, path) (NULL) #define get_child_json(e, name) (NULL) +#define iterate_field_json(e, cb, data) (0) #define get_field_json(e, path, dest) #define json_object_object_get_ex(a,b,c) (0) #define json_object_array_get_idx(a, b) (0) @@ -82,6 +90,7 @@ void get_field_string_with_size(parsertype p, void *e, const char *path, int get_array_length(parsertype p, void *root); void *get_elem_from_idx(parsertype p, void *node, int idx); void *get_child(parsertype p, void *node, const char *name); +void iterate_field(parsertype p, void *e, iterate_callback cb, void *data); void get_field(parsertype p, void *e, const char *path, void *dest); int exist_field_string(parsertype p, void *e, const char *path); void get_hash_value(parsertype p, void *elem, unsigned char *hash); diff --git a/parser/parser.c b/parser/parser.c index 44cd0fe..d4fcb11 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -152,32 +152,24 @@ static void *find_node(parsertype p, void *root, const char *node, return NULL; } -static void add_properties(parsertype p, void *node, struct img_type *image) +static void add_properties_cb(const char *name, const char *value, void *data) { + struct img_type *image = (struct img_type *)data; - void *properties, *prop; - int count, i; + TRACE("\t\tProperty %s: %s", name, value); + if (dict_insert_entry(&image->properties, (char *)name, (char *)value)) + ERROR("Property not stored, skipping..."); +} + +static void add_properties(parsertype p, void *node, struct img_type *image) +{ + void *properties; properties = get_child(p, node, "properties"); if (properties) { - count = get_array_length(p, properties); - - TRACE("Found %d properties for %s:", count, image->fname); - - for (i = 0; i < count; i++) { - char key[255]; - char value[255]; - prop = get_elem_from_idx(p, properties, i); - GET_FIELD_STRING(p, prop, "name", key); - GET_FIELD_STRING(p, prop, "value", value); - TRACE("\t\tProperty %d: name=%s val=%s ", i, - key, - value - ); - if (dict_insert_entry(&image->properties, key, value)) - ERROR("Property not stored, skipping..."); + TRACE("Found properties for %s:", image->fname); - } + iterate_field(p, properties, add_properties_cb, image); } }