| Message ID | 20181114160221.18242-5-sbabic@denx.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Support for links into sw-description. | expand |
Am 14.11.18 um 17:02 schrieb Stefano Babic: > There are a lot of use cases when special configurations are needed. > Current sw-description requires that each entry is full described and > does not allow to factorize some parts. This means that in most cases > entries are duplicated multiple times even if changes are minimal > between sections. > > This patch introduce links for the root node. If the node is a string > instead of an object, it is interpreted as new link to be follow. Doesn't this describes the old behavior? > The parser tries to follow the links recursively (a maximum number of 10 > links is introduce to avoid deadlocks). > > The patch factorizes how to call find_node() for the specific parser. > Instead of using two mechanism (a formatted sting and an array of > strings) for libconfig and json, use an array of strings for both. > > Signed-off-by: Stefano Babic <sbabic@denx.de> > --- > > Changes in v2: None > > corelib/parsing_library.c | 137 ++++++++++++----- > corelib/parsing_library_libconfig.c | 71 +++++++++ > corelib/parsing_library_libjson.c | 37 +++++ > include/parselib.h | 10 +- > parser/parser.c | 219 +++++++++++++--------------- > 5 files changed, 320 insertions(+), 154 deletions(-) > > diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c > index 00463a7..fd451ac 100644 > --- a/corelib/parsing_library.c > +++ b/corelib/parsing_library.c [snip] > @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, unsigned char *hash) > > bool set_find_path(const char **nodes, const char *newpath, char **tmp) > { > - unsigned int nleading; > - char **iter, **paths; > - unsigned int count = count_string_array(nodes); > - unsigned int countpaths; > - > - if (!newpath) > - return false; > + char **paths; > + unsigned int count; > + char *saveptr; > + char *token, *ref; > + bool first = true; > + int allocstr = 0; > > /* > - * Check if we have to traverse back > + * Include of files is not supported, > + * each reference should start with '#' > */ > - for (nleading = 0; newpath[nleading] == '.'; nleading++); > - > - /* > - * delimiter at the beginning indicates a relative path > - * exactly as in Unix, that mean .. for the upper directory > - * .. = parent > - * .... = parent of parent > - * The number of leading "." must be even, else > - * it is a malformed path > - */ > - if (nleading % 2) > + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) > return false; > > - nleading /= 2; > - if ((count - nleading) <= 0) > + ref = strdup(newpath); > + if (!ref) { > + ERROR("No memory: failed for %lu bytes", > + strlen(newpath) + 1); > return false; > - > - count -= nleading; > - if (count > 0) count--; > - > - paths = string_split(newpath, '.'); > + } > > /* > - * check if there is enough space in nodes > + * First find how many strings should be stored > + * Each token is stored temporarily on the heap > */ > - countpaths = count_string_array((const char **)paths); > - if (count + countpaths >= MAX_PARSED_NODES) > + count = 0; > + token = strtok_r(&ref[1], "/", &saveptr); > + while (token) { > + count++; > + token = strtok_r(NULL, "/", &saveptr); > + } > + free(ref); /* string was changed, clean and copy again */ > + > + if (!count) > + return false; > + > + paths = calloc(count + 1, sizeof(char*) * count); > + if (!paths) { > + ERROR("No memory: calloc failed for %lu bytes", > + sizeof(char*) * count); > + return false; > + } > + count = count_string_array(nodes); > + ref = strdup(newpath); > + if (!ref) { > + ERROR("No memory: failed for %lu bytes", > + strlen(newpath) + 1); > + free(paths); > return false; > - if (!countpaths) > - nodes[count++] = newpath; > - else > - for (iter = paths; *iter != NULL; iter++, count++) > - nodes[count] = *iter; > - nodes[count] = NULL; > + } > + > + token = strtok_r(&ref[1], "/", &saveptr); > + while (token) { > + if (!strcmp(token, "..")) { > + if (!count) { > + free(ref); > + free_string_array(paths); > + return false; > + } > + count--; > + } else if (strcmp(token, ".")) { > + if (first) { > + count = 0; > + } > + paths[allocstr] = strdup(token); > + nodes[count++] = paths[allocstr++]; > + paths[allocstr] = NULL; > + nodes[count] = NULL; > + if (count >= MAX_PARSED_NODES) { > + ERROR("Big depth in link, giving up..."); > + free_string_array(paths); > + free(ref); > + return false; > + } > + } > + token = strtok_r(NULL, "/", &saveptr); > + first = false; > + } > > + free(ref); > tmp = paths; > > return true; Please move this in PATCH 3/5. I wonder if a list would be better than an array. > diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c > index 415116b..d3fc3c2 100644 > --- a/corelib/parsing_library_libconfig.c > +++ b/corelib/parsing_library_libconfig.c > @@ -13,12 +13,34 @@ > #include <errno.h> > #include <sys/stat.h> > #include <assert.h> > +#include <stdbool.h> > #include "generated/autoconf.h" > #include "bsdqueue.h" > #include "util.h" > #include "swupdate.h" > #include "parselib.h" > > +static void path_libconfig(const char **nodes, char *root, unsigned int rootsize) > +{ > + const char **node; > + int nbytes, left; > + char *buf; > + const char *concat; > + bool first=true; > + > + root[0] = '\0'; > + > + for (node = nodes, buf = root, left = rootsize; *node != NULL; node++) { > + concat = first ? "" : "."; > + nbytes = snprintf(buf, left, "%s%s", concat, *node); > + buf += nbytes; > + left -= nbytes; > + first = false; > + if (left ==0) Space between == and 0. > + break; Maybe you should alloc the buffer inside the function: const char **node; size_t len = 0; char * buf; for (node = nodes, *node != NULL; node++) { len += strlen(node); len++; } buf = (char *)malloc(len); if (!buf) return NULL; buf[0] = '/0'; for (node = nodes, *node != NULL; node++) { strcat(buf, node); strcat(buf, "."); } buf[len] = '/0'; return buf; > + } > +} > + > void get_value_libconfig(const config_setting_t *e, void *dest) > { > int type = config_setting_type(e); > @@ -108,3 +130,52 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path) > > return NULL; > } > + > +void *get_node_libconfig(config_t *cfg, const char **nodes) > +{ > + config_setting_t *setting; > + char root[1024]; > + > + path_libconfig(nodes, root, sizeof(root)); > + setting = config_lookup(cfg, root); > + if (setting) > + return setting; > + > + return NULL; > +} > + > +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned int depth) > +{ > + config_setting_t *elem; > + char root[1024]; > + const char *ref; > + char **tmp = NULL; > + > + /* > + * check for deadlock links, block recursion > + */ > + if (!(--depth)) > + return NULL; > + > + path_libconfig(nodes, root, sizeof(root)); > + > + /* > + * If this is root node for the device, > + * it is a group and lenght is not 0. > + * If it is a link, follow it > + */ > + elem = config_lookup(cfg, root); > + > + if (elem && config_setting_is_group(elem) == CONFIG_TRUE) { > + ref = get_field_string_libconfig(elem, "ref"); > + if (ref) { > + if (!set_find_path(nodes, ref, tmp)) > + return NULL; > + elem = find_root_libconfig(cfg, nodes, depth); > + free_string_array(tmp); > + } > + } > + > + return elem; > + > +} > diff --git a/corelib/parsing_library_libjson.c b/corelib/parsing_library_libjson.c > index b1f5775..0322a4f 100644 > --- a/corelib/parsing_library_libjson.c > +++ b/corelib/parsing_library_libjson.c > @@ -7,6 +7,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <stdbool.h> > #include <sys/types.h> > #include <unistd.h> > #include <fcntl.h> > @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, const char *key) > : strndup(json_object_get_string(json_data), MAX_URL_LENGTH); > } > > +void *find_root_json(json_object *root, const char **nodes, unsigned int depth) > +{ > + json_object *node; > + enum json_type type; > + char **tmp = NULL; > + const char *str; > + > + /* > + * check for deadlock links, block recursion > + */ > + if (!(--depth)) > + return false; > + > + node = find_json_recursive_node(root, nodes); > + > + if (node) { > + type = json_object_get_type(node); > + > + if (type == json_type_object || type == json_type_array) { > + str = get_field_string_json(node, "ref"); > + if (str) { > + if (!set_find_path(nodes, str, tmp)) > + return NULL; > + node = find_root_json(root, nodes, depth); > + free_string_array(tmp); It isn't clear why you need the tmp. Isn't it possible to remove this requirement for the set_find_path function? > + } > + } > + } > + return node; > +} > + > +void *get_node_json(json_object *root, const char **nodes) > +{ > + return find_json_recursive_node(root, nodes); > +} > + > > diff --git a/include/parselib.h b/include/parselib.h > index 2fca3b5..5cbc255 100644 > --- a/include/parselib.h > +++ b/include/parselib.h > @@ -39,6 +39,8 @@ 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); > +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned int depth); > +void *get_node_libconfig(config_t *cfg, const char **nodes); > > #else > #define config_setting_get_elem(a,b) (NULL) > @@ -49,6 +51,8 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path); > #define get_child_libconfig(e, name) (NULL) > #define iterate_field_libconfig(e, cb, data) { } > #define get_field_cfg(e, path, dest) > +#define find_root_libconfig(cfg, nodes, depth) (NULL) > +#define get_node_libconfig(cfg, nodes) (NULL) > #endif > > #ifdef CONFIG_JSON > @@ -65,6 +69,8 @@ const char *json_get_value(struct json_object *json_root, > const char *key); > json_object *json_get_path_key(json_object *json_root, const char **json_path); > char *json_get_data_url(json_object *json_root, const char *key); > +void *find_root_json(json_object *root, const char **nodes, unsigned int depth); > +void *get_node_json(json_object *root, const char **nodes); > > #else > #define find_node_json(a, b, c) (NULL) > @@ -75,6 +81,8 @@ char *json_get_data_url(json_object *json_root, const char *key); > #define json_object_object_get_ex(a,b,c) (0) > #define json_object_array_get_idx(a, b) (0) > #define json_object_array_length(a) (0) > +#define find_root_json(root, nodes, depth) (NULL) > +#define get_node_json(root, nodes) (NULL) > #endif > > typedef int (*settings_callback)(void *elem, void *data); > @@ -90,7 +98,7 @@ 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); > void check_field_string(const char *src, char *dst, const size_t max_len); > -bool find_root(parsertype p, void *root, const char **nodes); > +void *find_root(parsertype p, void *root, const char **nodes); > void *get_node(parsertype p, void *root, const char **nodes); > bool set_find_path(const char **nodes, const char *newpath, char **tmp); > > diff --git a/parser/parser.c b/parser/parser.c > index 19181d1..bc89b9d 100644 > --- a/parser/parser.c > +++ b/parser/parser.c > @@ -11,6 +11,7 @@ > #include <string.h> > #include <sys/types.h> > #include <unistd.h> > +#include <stdbool.h> > #include <fcntl.h> > #include <errno.h> > #include <sys/stat.h> > @@ -29,115 +30,125 @@ > #define NODEROOT (!strlen(CONFIG_PARSERROOT) ? \ > "software" : CONFIG_PARSERROOT) > > -#ifdef CONFIG_LIBCONFIG > -static config_setting_t *find_node_libconfig(config_t *cfg, > - const char *field, struct swupdate_cfg *swcfg) > +#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) > + > +static bool path_append(const char **nodes, const char *field) > { > - config_setting_t *setting; > - struct hw_type *hardware; > + unsigned int count = 0; > > - char node[1024]; > + count = count_string_array(nodes); > > - if (!field) > - return NULL; > + if (count >= MAX_PARSED_NODES) > + return false; > > - hardware = &swcfg->hw; > + nodes[count++] = field; > + nodes[count] = NULL; > > - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { > - /* Try with both software set and board name */ > - if (strlen(hardware->boardname)) { > - snprintf(node, sizeof(node), "%s.%s.%s.%s.%s", > - NODEROOT, > - hardware->boardname, > - swcfg->software_set, > - swcfg->running_mode, > - field); > - setting = config_lookup(cfg, node); > - if (setting) > - return setting; > - } > - /* still here, try with software set and mode */ > - snprintf(node, sizeof(node), "%s.%s.%s.%s", > - NODEROOT, > - swcfg->software_set, > - swcfg->running_mode, > - field); > - setting = config_lookup(cfg, node); > - if (setting) > - return setting; > - > - } > - > - /* Try with board name */ > - if (strlen(hardware->boardname)) { > - snprintf(node, sizeof(node), "%s.%s.%s", > - NODEROOT, > - hardware->boardname, > - field); > - setting = config_lookup(cfg, node); > - if (setting) > - return setting; > - } > - /* Fall back without board entry */ > - snprintf(node, sizeof(node), "%s.%s", > - NODEROOT, > - field); > - return config_lookup(cfg, node); > + return true; > } > > -#endif > - > -#ifdef CONFIG_JSON > -static json_object *find_node_json(json_object *root, const char *node, > +static void *find_node(parsertype p, void *root, const char *field, > struct swupdate_cfg *swcfg) > { > - json_object *jnode = NULL; > - const char *simple_nodes[] = {NODEROOT, node, NULL}; > + > struct hw_type *hardware; > + const char **nodes; > + int i; > + > + if (!field) > + return NULL; > > hardware = &swcfg->hw; > > - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { > - if (strlen(hardware->boardname)) { > - const char *nodes[] = {NODEROOT, hardware->boardname, > - swcfg->software_set, swcfg->running_mode, > - node, NULL}; > - jnode = find_json_recursive_node(root, nodes); > - if (jnode) > - return jnode; > - } else { > - const char *nodes[] = {NODEROOT, swcfg->software_set, > - swcfg->running_mode, node, NULL}; > - jnode = find_json_recursive_node(root, nodes); > - if (jnode) > - return jnode; > + nodes = (const char **)calloc(MAX_PARSED_NODES, sizeof(*nodes)); > + > + for (i = 0; i < 4; i++) { > + nodes[0] = NULL; > + switch(i) { > + case 0: > + if (strlen(swcfg->running_mode) && strlen(swcfg->software_set) && > + strlen(hardware->boardname)) { > + nodes[0] = NODEROOT; > + nodes[1] = hardware->boardname; > + nodes[2] = swcfg->software_set; > + nodes[3] = swcfg->running_mode; > + nodes[4] = NULL; > + } > + break; > + case 1: > + /* try with software set and mode */ > + if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { > + nodes[0] = NODEROOT; > + nodes[1] = swcfg->software_set; > + nodes[2] = swcfg->running_mode; > + nodes[3] = NULL; > + } > + break; > + case 2: > + /* Try with board name */ > + if (strlen(hardware->boardname)) { > + nodes[0] = NODEROOT; > + nodes[1] = hardware->boardname; > + nodes[2] = NULL; > + } > + break; > + case 3: > + /* Fall back without board entry */ > + nodes[0] = NODEROOT; > + nodes[1] = NULL; > + break; > } > - } > > - if (strlen(hardware->boardname)) { > - const char *nodes[] = {NODEROOT, hardware->boardname, node, > - NULL}; > - jnode = find_json_recursive_node(root, nodes); > - if (jnode) > - return jnode; > + /* > + * If conditions are not set, > + * skip to the next option > + */ > + if (!nodes[0]) > + continue; > + > + /* > + * The first find_root() search for > + * the root element from board, selection > + * The second one starts from root and follow the tree > + * to search for element > + */ > + if (find_root(p, root, nodes)) { > + void *node = NULL; > + if (!path_append(nodes, field)) > + return NULL; > + node = find_root(p, root, nodes); > + > + if (node) { > + free(nodes); > + return node; > + } > + } > } > > - return find_json_recursive_node(root, simple_nodes); > + free(nodes); > + > + return NULL; > } > -#endif > > -#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) > -static void *find_node(parsertype p, void *root, const char *node, > - struct swupdate_cfg *swcfg) > +static bool get_common_fields(parsertype p, void *cfg, struct swupdate_cfg *swcfg) > { > - switch (p) { > - case LIBCFG_PARSER: > - return find_node_libconfig((config_t *)root, node, swcfg); > - case JSON_PARSER: > - return find_node_json((json_object *)root, node, swcfg); > + > + void *setting; > + > + if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { > + ERROR("Missing version in configuration file"); > + return false; > } > + > + GET_FIELD_STRING(p, setting, NULL, swcfg->version); > + TRACE("Version %s", swcfg->version); > > - return NULL; > + if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { > + GET_FIELD_STRING(p, setting, NULL, swcfg->description); > + TRACE("Description %s", swcfg->description); > + } > + > + return true; > } > > static void add_properties_cb(const char *name, const char *value, void *data) > @@ -672,11 +683,8 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg) > int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) > { > config_t cfg; > - const char *str; > - char node[128]; > parsertype p = LIBCFG_PARSER; > int ret; > - config_setting_t *setting; > > memset(&cfg, 0, sizeof(cfg)); > config_init(&cfg); > @@ -694,24 +702,8 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) > return -1; > } > > - if((setting = find_node(p, &cfg, "version", swcfg)) == NULL) { > - ERROR("Missing version in configuration file"); > + if (!get_common_fields(p, &cfg, swcfg)) > return -1; > - } else { > - GET_FIELD_STRING(p, setting, NULL, swcfg->version); > - TRACE("Version %s", swcfg->version); > - } > - > - if((setting = find_node(p, &cfg, "description", swcfg)) != NULL) { > - GET_FIELD_STRING(p, setting, NULL, swcfg->description); > - TRACE("Description %s", swcfg->description); > - } > - > - snprintf(node, sizeof(node), "%s.embedded-script", > - NODEROOT); > - if (config_lookup_string(&cfg, node, &str)) { > - TRACE("Found Lua Software:\n%s", str); > - } > > ret = parser(p, &cfg, swcfg); > > @@ -734,7 +726,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) > struct stat stbuf; > unsigned int size; > char *string; > - json_object *cfg, *setting; > + json_object *cfg; > parsertype p = JSON_PARSER; > > /* Read the file. If there is an error, report it and exit. */ > @@ -764,18 +756,9 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) > return -1; > } > > - if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { > - ERROR("Missing version in configuration file"); > + if (!get_common_fields(p, cfg, swcfg)) { > free(string); > return -1; > - } else { > - GET_FIELD_STRING(p, setting, NULL, swcfg->version); > - TRACE("Version %s", swcfg->version); > - } > - > - if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { > - GET_FIELD_STRING(p, setting, NULL, swcfg->description); > - TRACE("Description %s", swcfg->description); > } > > ret = parser(p, cfg, swcfg); > Best regards Stefan
Hi Stefan, On 23/11/18 23:42, Stefan Herbrechtsmeier wrote: > Am 14.11.18 um 17:02 schrieb Stefano Babic: >> There are a lot of use cases when special configurations are needed. >> Current sw-description requires that each entry is full described and >> does not allow to factorize some parts. This means that in most cases >> entries are duplicated multiple times even if changes are minimal >> between sections. >> >> This patch introduce links for the root node. If the node is a string >> instead of an object, it is interpreted as new link to be follow. > > Doesn't this describes the old behavior? > Thanks - I fix it. >> The parser tries to follow the links recursively (a maximum number of 10 >> links is introduce to avoid deadlocks). >> >> The patch factorizes how to call find_node() for the specific parser. >> Instead of using two mechanism (a formatted sting and an array of >> strings) for libconfig and json, use an array of strings for both. >> >> Signed-off-by: Stefano Babic <sbabic@denx.de> >> --- >> >> Changes in v2: None >> >> corelib/parsing_library.c | 137 ++++++++++++----- >> corelib/parsing_library_libconfig.c | 71 +++++++++ >> corelib/parsing_library_libjson.c | 37 +++++ >> include/parselib.h | 10 +- >> parser/parser.c | 219 +++++++++++++--------------- >> 5 files changed, 320 insertions(+), 154 deletions(-) >> >> diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c >> index 00463a7..fd451ac 100644 >> --- a/corelib/parsing_library.c >> +++ b/corelib/parsing_library.c > > [snip] > >> @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, >> unsigned char *hash) >> bool set_find_path(const char **nodes, const char *newpath, char >> **tmp) >> { >> - unsigned int nleading; >> - char **iter, **paths; >> - unsigned int count = count_string_array(nodes); >> - unsigned int countpaths; >> - >> - if (!newpath) >> - return false; >> + char **paths; >> + unsigned int count; >> + char *saveptr; >> + char *token, *ref; >> + bool first = true; >> + int allocstr = 0; >> /* >> - * Check if we have to traverse back >> + * Include of files is not supported, >> + * each reference should start with '#' >> */ >> - for (nleading = 0; newpath[nleading] == '.'; nleading++); >> - >> - /* >> - * delimiter at the beginning indicates a relative path >> - * exactly as in Unix, that mean .. for the upper directory >> - * .. = parent >> - * .... = parent of parent >> - * The number of leading "." must be even, else >> - * it is a malformed path >> - */ >> - if (nleading % 2) >> + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) >> return false; >> - nleading /= 2; >> - if ((count - nleading) <= 0) >> + ref = strdup(newpath); >> + if (!ref) { >> + ERROR("No memory: failed for %lu bytes", >> + strlen(newpath) + 1); >> return false; >> - >> - count -= nleading; >> - if (count > 0) count--; >> - >> - paths = string_split(newpath, '.'); >> + } >> /* >> - * check if there is enough space in nodes >> + * First find how many strings should be stored >> + * Each token is stored temporarily on the heap >> */ >> - countpaths = count_string_array((const char **)paths); >> - if (count + countpaths >= MAX_PARSED_NODES) >> + count = 0; >> + token = strtok_r(&ref[1], "/", &saveptr); >> + while (token) { >> + count++; >> + token = strtok_r(NULL, "/", &saveptr); >> + } >> + free(ref); /* string was changed, clean and copy again */ >> + >> + if (!count) >> + return false; >> + >> + paths = calloc(count + 1, sizeof(char*) * count); >> + if (!paths) { >> + ERROR("No memory: calloc failed for %lu bytes", >> + sizeof(char*) * count); >> + return false; >> + } >> + count = count_string_array(nodes); >> + ref = strdup(newpath); >> + if (!ref) { >> + ERROR("No memory: failed for %lu bytes", >> + strlen(newpath) + 1); >> + free(paths); >> return false; >> - if (!countpaths) >> - nodes[count++] = newpath; >> - else >> - for (iter = paths; *iter != NULL; iter++, count++) >> - nodes[count] = *iter; >> - nodes[count] = NULL; >> + } >> + >> + token = strtok_r(&ref[1], "/", &saveptr); >> + while (token) { >> + if (!strcmp(token, "..")) { >> + if (!count) { >> + free(ref); >> + free_string_array(paths); >> + return false; >> + } >> + count--; >> + } else if (strcmp(token, ".")) { >> + if (first) { >> + count = 0; >> + } >> + paths[allocstr] = strdup(token); >> + nodes[count++] = paths[allocstr++]; >> + paths[allocstr] = NULL; >> + nodes[count] = NULL; >> + if (count >= MAX_PARSED_NODES) { >> + ERROR("Big depth in link, giving up..."); >> + free_string_array(paths); >> + free(ref); >> + return false; >> + } >> + } >> + token = strtok_r(NULL, "/", &saveptr); >> + first = false; >> + } >> + free(ref); >> tmp = paths; >> return true; > > Please move this in PATCH 3/5. > > I wonder if a list would be better than an array. Agree that a list is better, but I didn't want to do all in one shot. To add the same feature to both libconfig and json parser, they have to use quite the same structures. libconfig had its own and json uses an array of char pointers. My first step was to unify both and my approach was to move libconfig to have the same as json, that means an array. A doubled-linked list will be the next step for this. > >> diff --git a/corelib/parsing_library_libconfig.c >> b/corelib/parsing_library_libconfig.c >> index 415116b..d3fc3c2 100644 >> --- a/corelib/parsing_library_libconfig.c >> +++ b/corelib/parsing_library_libconfig.c >> @@ -13,12 +13,34 @@ >> #include <errno.h> >> #include <sys/stat.h> >> #include <assert.h> >> +#include <stdbool.h> >> #include "generated/autoconf.h" >> #include "bsdqueue.h" >> #include "util.h" >> #include "swupdate.h" >> #include "parselib.h" >> +static void path_libconfig(const char **nodes, char *root, unsigned >> int rootsize) >> +{ >> + const char **node; >> + int nbytes, left; >> + char *buf; >> + const char *concat; >> + bool first=true; >> + >> + root[0] = '\0'; >> + >> + for (node = nodes, buf = root, left = rootsize; *node != NULL; >> node++) { >> + concat = first ? "" : "."; >> + nbytes = snprintf(buf, left, "%s%s", concat, *node); >> + buf += nbytes; >> + left -= nbytes; >> + first = false; >> + if (left ==0) > > Space between == and 0. > >> + break; > I fix it, thanks. > Maybe you should alloc the buffer inside the function: > mmhhh...you're right. The "root" parameter is nasty because it is not needed by the caller. > const char **node; > size_t len = 0; > char * buf; > > for (node = nodes, *node != NULL; node++) { > len += strlen(node); > len++; > } > > buf = (char *)malloc(len); > if (!buf) > return NULL; > > buf[0] = '/0'; > for (node = nodes, *node != NULL; node++) { > strcat(buf, node); > strcat(buf, "."); > } > buf[len] = '/0'; > > return buf; > > >> + } >> +} >> + >> void get_value_libconfig(const config_setting_t *e, void *dest) >> { >> int type = config_setting_type(e); >> @@ -108,3 +130,52 @@ const char >> *get_field_string_libconfig(config_setting_t *e, const char *path) >> return NULL; >> } >> + >> +void *get_node_libconfig(config_t *cfg, const char **nodes) >> +{ >> + config_setting_t *setting; >> + char root[1024]; >> + >> + path_libconfig(nodes, root, sizeof(root)); >> + setting = config_lookup(cfg, root); >> + if (setting) >> + return setting; >> + >> + return NULL; >> +} >> + >> +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned >> int depth) >> +{ >> + config_setting_t *elem; >> + char root[1024]; >> + const char *ref; >> + char **tmp = NULL; >> + >> + /* >> + * check for deadlock links, block recursion >> + */ >> + if (!(--depth)) >> + return NULL; >> + >> + path_libconfig(nodes, root, sizeof(root)); >> + >> + /* >> + * If this is root node for the device, >> + * it is a group and lenght is not 0. >> + * If it is a link, follow it >> + */ >> + elem = config_lookup(cfg, root); >> + >> + if (elem && config_setting_is_group(elem) == CONFIG_TRUE) { >> + ref = get_field_string_libconfig(elem, "ref"); >> + if (ref) { >> + if (!set_find_path(nodes, ref, tmp)) >> + return NULL; >> + elem = find_root_libconfig(cfg, nodes, depth); >> + free_string_array(tmp); >> + } >> + } >> + >> + return elem; >> + >> +} >> diff --git a/corelib/parsing_library_libjson.c >> b/corelib/parsing_library_libjson.c >> index b1f5775..0322a4f 100644 >> --- a/corelib/parsing_library_libjson.c >> +++ b/corelib/parsing_library_libjson.c >> @@ -7,6 +7,7 @@ >> #include <stdio.h> >> #include <stdlib.h> >> #include <string.h> >> +#include <stdbool.h> >> #include <sys/types.h> >> #include <unistd.h> >> #include <fcntl.h> >> @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, >> const char *key) >> : strndup(json_object_get_string(json_data), >> MAX_URL_LENGTH); >> } >> +void *find_root_json(json_object *root, const char **nodes, >> unsigned int depth) >> +{ >> + json_object *node; >> + enum json_type type; >> + char **tmp = NULL; >> + const char *str; >> + >> + /* >> + * check for deadlock links, block recursion >> + */ >> + if (!(--depth)) >> + return false; >> + >> + node = find_json_recursive_node(root, nodes); >> + >> + if (node) { >> + type = json_object_get_type(node); >> + >> + if (type == json_type_object || type == json_type_array) { >> + str = get_field_string_json(node, "ref"); >> + if (str) { >> + if (!set_find_path(nodes, str, tmp)) >> + return NULL; >> + node = find_root_json(root, nodes, depth); >> + free_string_array(tmp); > > It isn't clear why you need the tmp. Isn't it possible to remove this > requirement for the set_find_path function? This comes from the choice to have an array of char**, and the caller is the allocator. I guess it can be removed after switching to a list. > >> + } >> + } >> + } >> + return node; >> +} >> + >> +void *get_node_json(json_object *root, const char **nodes) >> +{ >> + return find_json_recursive_node(root, nodes); >> +} >> + >> diff --git a/include/parselib.h b/include/parselib.h >> index 2fca3b5..5cbc255 100644 >> --- a/include/parselib.h >> +++ b/include/parselib.h >> @@ -39,6 +39,8 @@ 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); >> +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned >> int depth); >> +void *get_node_libconfig(config_t *cfg, const char **nodes); >> #else >> #define config_setting_get_elem(a,b) (NULL) >> @@ -49,6 +51,8 @@ const char >> *get_field_string_libconfig(config_setting_t *e, const char *path); >> #define get_child_libconfig(e, name) (NULL) >> #define iterate_field_libconfig(e, cb, data) { } >> #define get_field_cfg(e, path, dest) >> +#define find_root_libconfig(cfg, nodes, depth) (NULL) >> +#define get_node_libconfig(cfg, nodes) (NULL) >> #endif >> #ifdef CONFIG_JSON >> @@ -65,6 +69,8 @@ const char *json_get_value(struct json_object >> *json_root, >> const char *key); >> json_object *json_get_path_key(json_object *json_root, const char >> **json_path); >> char *json_get_data_url(json_object *json_root, const char *key); >> +void *find_root_json(json_object *root, const char **nodes, unsigned >> int depth); >> +void *get_node_json(json_object *root, const char **nodes); >> #else >> #define find_node_json(a, b, c) (NULL) >> @@ -75,6 +81,8 @@ char *json_get_data_url(json_object *json_root, >> const char *key); >> #define json_object_object_get_ex(a,b,c) (0) >> #define json_object_array_get_idx(a, b) (0) >> #define json_object_array_length(a) (0) >> +#define find_root_json(root, nodes, depth) (NULL) >> +#define get_node_json(root, nodes) (NULL) >> #endif >> typedef int (*settings_callback)(void *elem, void *data); >> @@ -90,7 +98,7 @@ 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); >> void check_field_string(const char *src, char *dst, const size_t >> max_len); >> -bool find_root(parsertype p, void *root, const char **nodes); >> +void *find_root(parsertype p, void *root, const char **nodes); >> void *get_node(parsertype p, void *root, const char **nodes); >> bool set_find_path(const char **nodes, const char *newpath, char >> **tmp); >> diff --git a/parser/parser.c b/parser/parser.c >> index 19181d1..bc89b9d 100644 >> --- a/parser/parser.c >> +++ b/parser/parser.c >> @@ -11,6 +11,7 @@ >> #include <string.h> >> #include <sys/types.h> >> #include <unistd.h> >> +#include <stdbool.h> >> #include <fcntl.h> >> #include <errno.h> >> #include <sys/stat.h> >> @@ -29,115 +30,125 @@ >> #define NODEROOT (!strlen(CONFIG_PARSERROOT) ? \ >> "software" : CONFIG_PARSERROOT) >> -#ifdef CONFIG_LIBCONFIG >> -static config_setting_t *find_node_libconfig(config_t *cfg, >> - const char *field, struct swupdate_cfg *swcfg) >> +#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) >> + >> +static bool path_append(const char **nodes, const char *field) >> { >> - config_setting_t *setting; >> - struct hw_type *hardware; >> + unsigned int count = 0; >> - char node[1024]; >> + count = count_string_array(nodes); >> - if (!field) >> - return NULL; >> + if (count >= MAX_PARSED_NODES) >> + return false; >> - hardware = &swcfg->hw; >> + nodes[count++] = field; >> + nodes[count] = NULL; >> - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { >> - /* Try with both software set and board name */ >> - if (strlen(hardware->boardname)) { >> - snprintf(node, sizeof(node), "%s.%s.%s.%s.%s", >> - NODEROOT, >> - hardware->boardname, >> - swcfg->software_set, >> - swcfg->running_mode, >> - field); >> - setting = config_lookup(cfg, node); >> - if (setting) >> - return setting; >> - } >> - /* still here, try with software set and mode */ >> - snprintf(node, sizeof(node), "%s.%s.%s.%s", >> - NODEROOT, >> - swcfg->software_set, >> - swcfg->running_mode, >> - field); >> - setting = config_lookup(cfg, node); >> - if (setting) >> - return setting; >> - >> - } >> - >> - /* Try with board name */ >> - if (strlen(hardware->boardname)) { >> - snprintf(node, sizeof(node), "%s.%s.%s", >> - NODEROOT, >> - hardware->boardname, >> - field); >> - setting = config_lookup(cfg, node); >> - if (setting) >> - return setting; >> - } >> - /* Fall back without board entry */ >> - snprintf(node, sizeof(node), "%s.%s", >> - NODEROOT, >> - field); >> - return config_lookup(cfg, node); >> + return true; >> } >> -#endif >> - >> -#ifdef CONFIG_JSON >> -static json_object *find_node_json(json_object *root, const char *node, >> +static void *find_node(parsertype p, void *root, const char *field, >> struct swupdate_cfg *swcfg) >> { >> - json_object *jnode = NULL; >> - const char *simple_nodes[] = {NODEROOT, node, NULL}; >> + >> struct hw_type *hardware; >> + const char **nodes; >> + int i; >> + >> + if (!field) >> + return NULL; >> hardware = &swcfg->hw; >> - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { >> - if (strlen(hardware->boardname)) { >> - const char *nodes[] = {NODEROOT, hardware->boardname, >> - swcfg->software_set, swcfg->running_mode, >> - node, NULL}; >> - jnode = find_json_recursive_node(root, nodes); >> - if (jnode) >> - return jnode; >> - } else { >> - const char *nodes[] = {NODEROOT, swcfg->software_set, >> - swcfg->running_mode, node, NULL}; >> - jnode = find_json_recursive_node(root, nodes); >> - if (jnode) >> - return jnode; >> + nodes = (const char **)calloc(MAX_PARSED_NODES, sizeof(*nodes)); >> + >> + for (i = 0; i < 4; i++) { >> + nodes[0] = NULL; >> + switch(i) { >> + case 0: >> + if (strlen(swcfg->running_mode) && >> strlen(swcfg->software_set) && >> + strlen(hardware->boardname)) { >> + nodes[0] = NODEROOT; >> + nodes[1] = hardware->boardname; >> + nodes[2] = swcfg->software_set; >> + nodes[3] = swcfg->running_mode; >> + nodes[4] = NULL; >> + } >> + break; >> + case 1: >> + /* try with software set and mode */ >> + if (strlen(swcfg->running_mode) && >> strlen(swcfg->software_set)) { >> + nodes[0] = NODEROOT; >> + nodes[1] = swcfg->software_set; >> + nodes[2] = swcfg->running_mode; >> + nodes[3] = NULL; >> + } >> + break; >> + case 2: >> + /* Try with board name */ >> + if (strlen(hardware->boardname)) { >> + nodes[0] = NODEROOT; >> + nodes[1] = hardware->boardname; >> + nodes[2] = NULL; >> + } >> + break; >> + case 3: >> + /* Fall back without board entry */ >> + nodes[0] = NODEROOT; >> + nodes[1] = NULL; >> + break; >> } >> - } >> - if (strlen(hardware->boardname)) { >> - const char *nodes[] = {NODEROOT, hardware->boardname, node, >> - NULL}; >> - jnode = find_json_recursive_node(root, nodes); >> - if (jnode) >> - return jnode; >> + /* >> + * If conditions are not set, >> + * skip to the next option >> + */ >> + if (!nodes[0]) >> + continue; >> + >> + /* >> + * The first find_root() search for >> + * the root element from board, selection >> + * The second one starts from root and follow the tree >> + * to search for element >> + */ >> + if (find_root(p, root, nodes)) { >> + void *node = NULL; >> + if (!path_append(nodes, field)) >> + return NULL; >> + node = find_root(p, root, nodes); >> + >> + if (node) { >> + free(nodes); >> + return node; >> + } >> + } >> } >> - return find_json_recursive_node(root, simple_nodes); >> + free(nodes); >> + >> + return NULL; >> } >> -#endif >> -#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) >> -static void *find_node(parsertype p, void *root, const char *node, >> - struct swupdate_cfg *swcfg) >> +static bool get_common_fields(parsertype p, void *cfg, struct >> swupdate_cfg *swcfg) >> { >> - switch (p) { >> - case LIBCFG_PARSER: >> - return find_node_libconfig((config_t *)root, node, swcfg); >> - case JSON_PARSER: >> - return find_node_json((json_object *)root, node, swcfg); >> + >> + void *setting; >> + >> + if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { >> + ERROR("Missing version in configuration file"); >> + return false; >> } >> + >> + GET_FIELD_STRING(p, setting, NULL, swcfg->version); >> + TRACE("Version %s", swcfg->version); >> - return NULL; >> + if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { >> + GET_FIELD_STRING(p, setting, NULL, swcfg->description); >> + TRACE("Description %s", swcfg->description); >> + } >> + >> + return true; >> } >> static void add_properties_cb(const char *name, const char *value, >> void *data) >> @@ -672,11 +683,8 @@ static int parser(parsertype p, void *cfg, struct >> swupdate_cfg *swcfg) >> int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) >> { >> config_t cfg; >> - const char *str; >> - char node[128]; >> parsertype p = LIBCFG_PARSER; >> int ret; >> - config_setting_t *setting; >> memset(&cfg, 0, sizeof(cfg)); >> config_init(&cfg); >> @@ -694,24 +702,8 @@ int parse_cfg (struct swupdate_cfg *swcfg, const >> char *filename) >> return -1; >> } >> - if((setting = find_node(p, &cfg, "version", swcfg)) == NULL) { >> - ERROR("Missing version in configuration file"); >> + if (!get_common_fields(p, &cfg, swcfg)) >> return -1; >> - } else { >> - GET_FIELD_STRING(p, setting, NULL, swcfg->version); >> - TRACE("Version %s", swcfg->version); >> - } >> - >> - if((setting = find_node(p, &cfg, "description", swcfg)) != NULL) { >> - GET_FIELD_STRING(p, setting, NULL, swcfg->description); >> - TRACE("Description %s", swcfg->description); >> - } >> - >> - snprintf(node, sizeof(node), "%s.embedded-script", >> - NODEROOT); >> - if (config_lookup_string(&cfg, node, &str)) { >> - TRACE("Found Lua Software:\n%s", str); >> - } >> ret = parser(p, &cfg, swcfg); >> @@ -734,7 +726,7 @@ int parse_json(struct swupdate_cfg *swcfg, const >> char *filename) >> struct stat stbuf; >> unsigned int size; >> char *string; >> - json_object *cfg, *setting; >> + json_object *cfg; >> parsertype p = JSON_PARSER; >> /* Read the file. If there is an error, report it and exit. */ >> @@ -764,18 +756,9 @@ int parse_json(struct swupdate_cfg *swcfg, const >> char *filename) >> return -1; >> } >> - if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { >> - ERROR("Missing version in configuration file"); >> + if (!get_common_fields(p, cfg, swcfg)) { >> free(string); >> return -1; >> - } else { >> - GET_FIELD_STRING(p, setting, NULL, swcfg->version); >> - TRACE("Version %s", swcfg->version); >> - } >> - >> - if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { >> - GET_FIELD_STRING(p, setting, NULL, swcfg->description); >> - TRACE("Description %s", swcfg->description); >> } >> ret = parser(p, cfg, swcfg); >> > Best regards, Stefano
Hi Stefano, Am 24.11.18 um 10:40 schrieb Stefano Babic: > Hi Stefan, > > On 23/11/18 23:42, Stefan Herbrechtsmeier wrote: >> Am 14.11.18 um 17:02 schrieb Stefano Babic: >>> There are a lot of use cases when special configurations are needed. >>> Current sw-description requires that each entry is full described and >>> does not allow to factorize some parts. This means that in most cases >>> entries are duplicated multiple times even if changes are minimal >>> between sections. >>> >>> This patch introduce links for the root node. If the node is a string >>> instead of an object, it is interpreted as new link to be follow. >> >> Doesn't this describes the old behavior? >> > > Thanks - I fix it. > >>> The parser tries to follow the links recursively (a maximum number of 10 >>> links is introduce to avoid deadlocks). >>> >>> The patch factorizes how to call find_node() for the specific parser. >>> Instead of using two mechanism (a formatted sting and an array of >>> strings) for libconfig and json, use an array of strings for both. >>> >>> Signed-off-by: Stefano Babic <sbabic@denx.de> >>> --- >>> >>> Changes in v2: None >>> >>> corelib/parsing_library.c | 137 ++++++++++++----- >>> corelib/parsing_library_libconfig.c | 71 +++++++++ >>> corelib/parsing_library_libjson.c | 37 +++++ >>> include/parselib.h | 10 +- >>> parser/parser.c | 219 +++++++++++++--------------- >>> 5 files changed, 320 insertions(+), 154 deletions(-) >>> >>> diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c >>> index 00463a7..fd451ac 100644 >>> --- a/corelib/parsing_library.c >>> +++ b/corelib/parsing_library.c >> >> [snip] >> >>> @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, >>> unsigned char *hash) >>> bool set_find_path(const char **nodes, const char *newpath, char >>> **tmp) >>> { >>> - unsigned int nleading; >>> - char **iter, **paths; >>> - unsigned int count = count_string_array(nodes); >>> - unsigned int countpaths; >>> - >>> - if (!newpath) >>> - return false; >>> + char **paths; >>> + unsigned int count; >>> + char *saveptr; >>> + char *token, *ref; >>> + bool first = true; >>> + int allocstr = 0; >>> /* >>> - * Check if we have to traverse back >>> + * Include of files is not supported, >>> + * each reference should start with '#' >>> */ >>> - for (nleading = 0; newpath[nleading] == '.'; nleading++); >>> - >>> - /* >>> - * delimiter at the beginning indicates a relative path >>> - * exactly as in Unix, that mean .. for the upper directory >>> - * .. = parent >>> - * .... = parent of parent >>> - * The number of leading "." must be even, else >>> - * it is a malformed path >>> - */ >>> - if (nleading % 2) >>> + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) >>> return false; >>> - nleading /= 2; >>> - if ((count - nleading) <= 0) >>> + ref = strdup(newpath); >>> + if (!ref) { >>> + ERROR("No memory: failed for %lu bytes", >>> + strlen(newpath) + 1); >>> return false; >>> - >>> - count -= nleading; >>> - if (count > 0) count--; >>> - >>> - paths = string_split(newpath, '.'); >>> + } >>> /* >>> - * check if there is enough space in nodes >>> + * First find how many strings should be stored >>> + * Each token is stored temporarily on the heap >>> */ >>> - countpaths = count_string_array((const char **)paths); >>> - if (count + countpaths >= MAX_PARSED_NODES) >>> + count = 0; >>> + token = strtok_r(&ref[1], "/", &saveptr); >>> + while (token) { >>> + count++; >>> + token = strtok_r(NULL, "/", &saveptr); >>> + } >>> + free(ref); /* string was changed, clean and copy again */ >>> + >>> + if (!count) >>> + return false; >>> + >>> + paths = calloc(count + 1, sizeof(char*) * count); >>> + if (!paths) { >>> + ERROR("No memory: calloc failed for %lu bytes", >>> + sizeof(char*) * count); >>> + return false; >>> + } >>> + count = count_string_array(nodes); >>> + ref = strdup(newpath); >>> + if (!ref) { >>> + ERROR("No memory: failed for %lu bytes", >>> + strlen(newpath) + 1); >>> + free(paths); >>> return false; >>> - if (!countpaths) >>> - nodes[count++] = newpath; >>> - else >>> - for (iter = paths; *iter != NULL; iter++, count++) >>> - nodes[count] = *iter; >>> - nodes[count] = NULL; >>> + } >>> + >>> + token = strtok_r(&ref[1], "/", &saveptr); >>> + while (token) { >>> + if (!strcmp(token, "..")) { >>> + if (!count) { >>> + free(ref); >>> + free_string_array(paths); >>> + return false; >>> + } >>> + count--; >>> + } else if (strcmp(token, ".")) { >>> + if (first) { >>> + count = 0; >>> + } >>> + paths[allocstr] = strdup(token); >>> + nodes[count++] = paths[allocstr++]; >>> + paths[allocstr] = NULL; >>> + nodes[count] = NULL; >>> + if (count >= MAX_PARSED_NODES) { >>> + ERROR("Big depth in link, giving up..."); >>> + free_string_array(paths); >>> + free(ref); >>> + return false; >>> + } >>> + } >>> + token = strtok_r(NULL, "/", &saveptr); >>> + first = false; >>> + } >>> + free(ref); >>> tmp = paths; >>> return true; >> >> Please move this in PATCH 3/5. >> >> I wonder if a list would be better than an array. > > Agree that a list is better, but I didn't want to do all in one shot. > > To add the same feature to both libconfig and json parser, they have to > use quite the same structures. libconfig had its own and json uses an > array of char pointers. My first step was to unify both and my approach > was to move libconfig to have the same as json, that means an array. > > A doubled-linked list will be the next step for this. I thought again about this and think that a string would be better. If the code use a normal path as string it could reuse common code or libraries like cwalk [1] to handle a file system like path. Furthermore JSON Pointers and other JSON libraries use this format and libconfig use a path string with '.' instead of '/'. [1] https://likle.github.io/cwalk/ The code could append a relative reference to the current path or use an absolute reference as path. Afterwards it normalize the path and eliminate all '../', './' or '//'. An additional printf like function with a format string and arguments could simplify the path generation. [snip] >>> diff --git a/corelib/parsing_library_libjson.c >>> b/corelib/parsing_library_libjson.c >>> index b1f5775..0322a4f 100644 >>> --- a/corelib/parsing_library_libjson.c >>> +++ b/corelib/parsing_library_libjson.c >>> @@ -7,6 +7,7 @@ >>> #include <stdio.h> >>> #include <stdlib.h> >>> #include <string.h> >>> +#include <stdbool.h> >>> #include <sys/types.h> >>> #include <unistd.h> >>> #include <fcntl.h> >>> @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, >>> const char *key) >>> : strndup(json_object_get_string(json_data), >>> MAX_URL_LENGTH); >>> } >>> +void *find_root_json(json_object *root, const char **nodes, >>> unsigned int depth) >>> +{ >>> + json_object *node; >>> + enum json_type type; >>> + char **tmp = NULL; >>> + const char *str; >>> + >>> + /* >>> + * check for deadlock links, block recursion >>> + */ >>> + if (!(--depth)) >>> + return false; >>> + >>> + node = find_json_recursive_node(root, nodes); >>> + >>> + if (node) { >>> + type = json_object_get_type(node); >>> + >>> + if (type == json_type_object || type == json_type_array) { >>> + str = get_field_string_json(node, "ref"); >>> + if (str) { >>> + if (!set_find_path(nodes, str, tmp)) >>> + return NULL; >>> + node = find_root_json(root, nodes, depth); >>> + free_string_array(tmp); >> >> It isn't clear why you need the tmp. Isn't it possible to remove this >> requirement for the set_find_path function? > > This comes from the choice to have an array of char**, and the caller is > the allocator. I guess it can be removed after switching to a list. I think in both cases you could free the unused strings in the function. I really think a string path would be better because it is much more simpler to handle even if you have to parse it during every normalization. > >> >>> + } >>> + } >>> + } >>> + return node; >>> +} >>> + >>> +void *get_node_json(json_object *root, const char **nodes) >>> +{ >>> + return find_json_recursive_node(root, nodes); >>> +} >>> + [snip] Best regards Stefan
Hi Stefan, On 25/11/18 15:40, Stefan Herbrechtsmeier wrote: > Hi Stefano, > > Am 24.11.18 um 10:40 schrieb Stefano Babic: >> Hi Stefan, >> >> On 23/11/18 23:42, Stefan Herbrechtsmeier wrote: >>> Am 14.11.18 um 17:02 schrieb Stefano Babic: >>>> There are a lot of use cases when special configurations are needed. >>>> Current sw-description requires that each entry is full described and >>>> does not allow to factorize some parts. This means that in most cases >>>> entries are duplicated multiple times even if changes are minimal >>>> between sections. >>>> >>>> This patch introduce links for the root node. If the node is a string >>>> instead of an object, it is interpreted as new link to be follow. >>> >>> Doesn't this describes the old behavior? >>> >> >> Thanks - I fix it. >> >>>> The parser tries to follow the links recursively (a maximum number >>>> of 10 >>>> links is introduce to avoid deadlocks). >>>> >>>> The patch factorizes how to call find_node() for the specific parser. >>>> Instead of using two mechanism (a formatted sting and an array of >>>> strings) for libconfig and json, use an array of strings for both. >>>> >>>> Signed-off-by: Stefano Babic <sbabic@denx.de> >>>> --- >>>> >>>> Changes in v2: None >>>> >>>> corelib/parsing_library.c | 137 ++++++++++++----- >>>> corelib/parsing_library_libconfig.c | 71 +++++++++ >>>> corelib/parsing_library_libjson.c | 37 +++++ >>>> include/parselib.h | 10 +- >>>> parser/parser.c | 219 >>>> +++++++++++++--------------- >>>> 5 files changed, 320 insertions(+), 154 deletions(-) >>>> >>>> diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c >>>> index 00463a7..fd451ac 100644 >>>> --- a/corelib/parsing_library.c >>>> +++ b/corelib/parsing_library.c >>> >>> [snip] >>> >>>> @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, >>>> unsigned char *hash) >>>> bool set_find_path(const char **nodes, const char *newpath, char >>>> **tmp) >>>> { >>>> - unsigned int nleading; >>>> - char **iter, **paths; >>>> - unsigned int count = count_string_array(nodes); >>>> - unsigned int countpaths; >>>> - >>>> - if (!newpath) >>>> - return false; >>>> + char **paths; >>>> + unsigned int count; >>>> + char *saveptr; >>>> + char *token, *ref; >>>> + bool first = true; >>>> + int allocstr = 0; >>>> /* >>>> - * Check if we have to traverse back >>>> + * Include of files is not supported, >>>> + * each reference should start with '#' >>>> */ >>>> - for (nleading = 0; newpath[nleading] == '.'; nleading++); >>>> - >>>> - /* >>>> - * delimiter at the beginning indicates a relative path >>>> - * exactly as in Unix, that mean .. for the upper directory >>>> - * .. = parent >>>> - * .... = parent of parent >>>> - * The number of leading "." must be even, else >>>> - * it is a malformed path >>>> - */ >>>> - if (nleading % 2) >>>> + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) >>>> return false; >>>> - nleading /= 2; >>>> - if ((count - nleading) <= 0) >>>> + ref = strdup(newpath); >>>> + if (!ref) { >>>> + ERROR("No memory: failed for %lu bytes", >>>> + strlen(newpath) + 1); >>>> return false; >>>> - >>>> - count -= nleading; >>>> - if (count > 0) count--; >>>> - >>>> - paths = string_split(newpath, '.'); >>>> + } >>>> /* >>>> - * check if there is enough space in nodes >>>> + * First find how many strings should be stored >>>> + * Each token is stored temporarily on the heap >>>> */ >>>> - countpaths = count_string_array((const char **)paths); >>>> - if (count + countpaths >= MAX_PARSED_NODES) >>>> + count = 0; >>>> + token = strtok_r(&ref[1], "/", &saveptr); >>>> + while (token) { >>>> + count++; >>>> + token = strtok_r(NULL, "/", &saveptr); >>>> + } >>>> + free(ref); /* string was changed, clean and copy again */ >>>> + >>>> + if (!count) >>>> + return false; >>>> + >>>> + paths = calloc(count + 1, sizeof(char*) * count); >>>> + if (!paths) { >>>> + ERROR("No memory: calloc failed for %lu bytes", >>>> + sizeof(char*) * count); >>>> + return false; >>>> + } >>>> + count = count_string_array(nodes); >>>> + ref = strdup(newpath); >>>> + if (!ref) { >>>> + ERROR("No memory: failed for %lu bytes", >>>> + strlen(newpath) + 1); >>>> + free(paths); >>>> return false; >>>> - if (!countpaths) >>>> - nodes[count++] = newpath; >>>> - else >>>> - for (iter = paths; *iter != NULL; iter++, count++) >>>> - nodes[count] = *iter; >>>> - nodes[count] = NULL; >>>> + } >>>> + >>>> + token = strtok_r(&ref[1], "/", &saveptr); >>>> + while (token) { >>>> + if (!strcmp(token, "..")) { >>>> + if (!count) { >>>> + free(ref); >>>> + free_string_array(paths); >>>> + return false; >>>> + } >>>> + count--; >>>> + } else if (strcmp(token, ".")) { >>>> + if (first) { >>>> + count = 0; >>>> + } >>>> + paths[allocstr] = strdup(token); >>>> + nodes[count++] = paths[allocstr++]; >>>> + paths[allocstr] = NULL; >>>> + nodes[count] = NULL; >>>> + if (count >= MAX_PARSED_NODES) { >>>> + ERROR("Big depth in link, giving up..."); >>>> + free_string_array(paths); >>>> + free(ref); >>>> + return false; >>>> + } >>>> + } >>>> + token = strtok_r(NULL, "/", &saveptr); >>>> + first = false; >>>> + } >>>> + free(ref); >>>> tmp = paths; >>>> return true; >>> >>> Please move this in PATCH 3/5. >>> >>> I wonder if a list would be better than an array. >> >> Agree that a list is better, but I didn't want to do all in one shot. >> >> To add the same feature to both libconfig and json parser, they have to >> use quite the same structures. libconfig had its own and json uses an >> array of char pointers. My first step was to unify both and my approach >> was to move libconfig to have the same as json, that means an array. >> >> A doubled-linked list will be the next step for this. > > I thought again about this and think that a string would be better. If > the code use a normal path as string it could reuse common code or > libraries like cwalk [1] to handle a file system like path. Furthermore > JSON Pointers and other JSON libraries use this format and libconfig use > a path string with '.' instead of '/'. Indeed, the difference in the tree traversing between the two parser is something annoying. First impülementation in V1 was more conform to libconfig, with several ".." to traverse up the tree. This second implementation uses JSON notation with "/" separator. I cannot satisfy both libraries at the same time, and I agree with you last comment that without a separator is misleading. If we need an additional library to link to SWUpdate, depends which new services we need. Currently, I just needed a few functions to provide the new path and as you have seen, I implemented myself. Of course, if we need much more or we reuse in other parts of SWUpdate, it makes sense to switch to a general library. > > [1] https://likle.github.io/cwalk/ > > The code could append a relative reference to the current path or use an > absolute reference as path. Afterwards it normalize the path and > eliminate all '../', './' or '//'. An additional printf like function > with a format string and arguments could simplify the path generation. > I will take a look, thanks. But I think this can be done as optimization and future enhancements and we do not need to switch now, if there are no big issues with the current implementation. I would like to push a release soon, and these enhancements can be done after release. > [snip] > >>>> diff --git a/corelib/parsing_library_libjson.c >>>> b/corelib/parsing_library_libjson.c >>>> index b1f5775..0322a4f 100644 >>>> --- a/corelib/parsing_library_libjson.c >>>> +++ b/corelib/parsing_library_libjson.c >>>> @@ -7,6 +7,7 @@ >>>> #include <stdio.h> >>>> #include <stdlib.h> >>>> #include <string.h> >>>> +#include <stdbool.h> >>>> #include <sys/types.h> >>>> #include <unistd.h> >>>> #include <fcntl.h> >>>> @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, >>>> const char *key) >>>> : strndup(json_object_get_string(json_data), >>>> MAX_URL_LENGTH); >>>> } >>>> +void *find_root_json(json_object *root, const char **nodes, >>>> unsigned int depth) >>>> +{ >>>> + json_object *node; >>>> + enum json_type type; >>>> + char **tmp = NULL; >>>> + const char *str; >>>> + >>>> + /* >>>> + * check for deadlock links, block recursion >>>> + */ >>>> + if (!(--depth)) >>>> + return false; >>>> + >>>> + node = find_json_recursive_node(root, nodes); >>>> + >>>> + if (node) { >>>> + type = json_object_get_type(node); >>>> + >>>> + if (type == json_type_object || type == json_type_array) { >>>> + str = get_field_string_json(node, "ref"); >>>> + if (str) { >>>> + if (!set_find_path(nodes, str, tmp)) >>>> + return NULL; >>>> + node = find_root_json(root, nodes, depth); >>>> + free_string_array(tmp); >>> >>> It isn't clear why you need the tmp. Isn't it possible to remove this >>> requirement for the set_find_path function? >> >> This comes from the choice to have an array of char**, and the caller is >> the allocator. I guess it can be removed after switching to a list. > > I think in both cases you could free the unused strings in the function. I take a look at this point. It is also what I do not like, but I had not found a better solution. > > I really think a string path would be better because it is much more > simpler to handle even if you have to parse it during every normalization. I think about, yes. C is not the best language to handle strings, and it is easy to introduce bugs or memory leaks. > >> >>> >>>> + } >>>> + } >>>> + } >>>> + return node; >>>> +} >>>> + >>>> +void *get_node_json(json_object *root, const char **nodes) >>>> +{ >>>> + return find_json_recursive_node(root, nodes); >>>> +} >>>> + > > [snip] > Best regards, Stefano
Hi Stefano, Am 26.11.18 um 11:39 schrieb Stefano Babic: > Hi Stefan, > > On 25/11/18 15:40, Stefan Herbrechtsmeier wrote: >> Hi Stefano, >> >> Am 24.11.18 um 10:40 schrieb Stefano Babic: >>> Hi Stefan, >>> >>> On 23/11/18 23:42, Stefan Herbrechtsmeier wrote: >>>> Am 14.11.18 um 17:02 schrieb Stefano Babic: >>>>> There are a lot of use cases when special configurations are needed. >>>>> Current sw-description requires that each entry is full described and >>>>> does not allow to factorize some parts. This means that in most cases >>>>> entries are duplicated multiple times even if changes are minimal >>>>> between sections. >>>>> >>>>> This patch introduce links for the root node. If the node is a string >>>>> instead of an object, it is interpreted as new link to be follow. >>>> >>>> Doesn't this describes the old behavior? >>>> >>> >>> Thanks - I fix it. >>> >>>>> The parser tries to follow the links recursively (a maximum number >>>>> of 10 >>>>> links is introduce to avoid deadlocks). >>>>> >>>>> The patch factorizes how to call find_node() for the specific parser. >>>>> Instead of using two mechanism (a formatted sting and an array of >>>>> strings) for libconfig and json, use an array of strings for both. >>>>> >>>>> Signed-off-by: Stefano Babic <sbabic@denx.de> >>>>> --- >>>>> >>>>> Changes in v2: None >>>>> >>>>> corelib/parsing_library.c | 137 ++++++++++++----- >>>>> corelib/parsing_library_libconfig.c | 71 +++++++++ >>>>> corelib/parsing_library_libjson.c | 37 +++++ >>>>> include/parselib.h | 10 +- >>>>> parser/parser.c | 219 >>>>> +++++++++++++--------------- >>>>> 5 files changed, 320 insertions(+), 154 deletions(-) >>>>> >>>>> diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c >>>>> index 00463a7..fd451ac 100644 >>>>> --- a/corelib/parsing_library.c >>>>> +++ b/corelib/parsing_library.c >>>> >>>> [snip] >>>> >>>>> @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, >>>>> unsigned char *hash) >>>>> bool set_find_path(const char **nodes, const char *newpath, char >>>>> **tmp) >>>>> { >>>>> - unsigned int nleading; >>>>> - char **iter, **paths; >>>>> - unsigned int count = count_string_array(nodes); >>>>> - unsigned int countpaths; >>>>> - >>>>> - if (!newpath) >>>>> - return false; >>>>> + char **paths; >>>>> + unsigned int count; >>>>> + char *saveptr; >>>>> + char *token, *ref; >>>>> + bool first = true; >>>>> + int allocstr = 0; >>>>> /* >>>>> - * Check if we have to traverse back >>>>> + * Include of files is not supported, >>>>> + * each reference should start with '#' >>>>> */ >>>>> - for (nleading = 0; newpath[nleading] == '.'; nleading++); >>>>> - >>>>> - /* >>>>> - * delimiter at the beginning indicates a relative path >>>>> - * exactly as in Unix, that mean .. for the upper directory >>>>> - * .. = parent >>>>> - * .... = parent of parent >>>>> - * The number of leading "." must be even, else >>>>> - * it is a malformed path >>>>> - */ >>>>> - if (nleading % 2) >>>>> + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) >>>>> return false; >>>>> - nleading /= 2; >>>>> - if ((count - nleading) <= 0) >>>>> + ref = strdup(newpath); >>>>> + if (!ref) { >>>>> + ERROR("No memory: failed for %lu bytes", >>>>> + strlen(newpath) + 1); >>>>> return false; >>>>> - >>>>> - count -= nleading; >>>>> - if (count > 0) count--; >>>>> - >>>>> - paths = string_split(newpath, '.'); >>>>> + } >>>>> /* >>>>> - * check if there is enough space in nodes >>>>> + * First find how many strings should be stored >>>>> + * Each token is stored temporarily on the heap >>>>> */ >>>>> - countpaths = count_string_array((const char **)paths); >>>>> - if (count + countpaths >= MAX_PARSED_NODES) >>>>> + count = 0; >>>>> + token = strtok_r(&ref[1], "/", &saveptr); >>>>> + while (token) { >>>>> + count++; >>>>> + token = strtok_r(NULL, "/", &saveptr); >>>>> + } >>>>> + free(ref); /* string was changed, clean and copy again */ >>>>> + >>>>> + if (!count) >>>>> + return false; >>>>> + >>>>> + paths = calloc(count + 1, sizeof(char*) * count); >>>>> + if (!paths) { >>>>> + ERROR("No memory: calloc failed for %lu bytes", >>>>> + sizeof(char*) * count); >>>>> + return false; >>>>> + } >>>>> + count = count_string_array(nodes); >>>>> + ref = strdup(newpath); >>>>> + if (!ref) { >>>>> + ERROR("No memory: failed for %lu bytes", >>>>> + strlen(newpath) + 1); >>>>> + free(paths); >>>>> return false; >>>>> - if (!countpaths) >>>>> - nodes[count++] = newpath; >>>>> - else >>>>> - for (iter = paths; *iter != NULL; iter++, count++) >>>>> - nodes[count] = *iter; >>>>> - nodes[count] = NULL; >>>>> + } >>>>> + >>>>> + token = strtok_r(&ref[1], "/", &saveptr); >>>>> + while (token) { >>>>> + if (!strcmp(token, "..")) { >>>>> + if (!count) { >>>>> + free(ref); >>>>> + free_string_array(paths); >>>>> + return false; >>>>> + } >>>>> + count--; >>>>> + } else if (strcmp(token, ".")) { >>>>> + if (first) { >>>>> + count = 0; >>>>> + } >>>>> + paths[allocstr] = strdup(token); >>>>> + nodes[count++] = paths[allocstr++]; >>>>> + paths[allocstr] = NULL; >>>>> + nodes[count] = NULL; >>>>> + if (count >= MAX_PARSED_NODES) { >>>>> + ERROR("Big depth in link, giving up..."); >>>>> + free_string_array(paths); >>>>> + free(ref); >>>>> + return false; >>>>> + } >>>>> + } >>>>> + token = strtok_r(NULL, "/", &saveptr); >>>>> + first = false; >>>>> + } >>>>> + free(ref); >>>>> tmp = paths; >>>>> return true; >>>> >>>> Please move this in PATCH 3/5. >>>> >>>> I wonder if a list would be better than an array. >>> >>> Agree that a list is better, but I didn't want to do all in one shot. >>> >>> To add the same feature to both libconfig and json parser, they have to >>> use quite the same structures. libconfig had its own and json uses an >>> array of char pointers. My first step was to unify both and my approach >>> was to move libconfig to have the same as json, that means an array. >>> >>> A doubled-linked list will be the next step for this. >> >> I thought again about this and think that a string would be better. If >> the code use a normal path as string it could reuse common code or >> libraries like cwalk [1] to handle a file system like path. Furthermore >> JSON Pointers and other JSON libraries use this format and libconfig use >> a path string with '.' instead of '/'. > > Indeed, the difference in the tree traversing between the two parser is > something annoying. First impülementation in V1 was more conform to > libconfig, with several ".." to traverse up the tree. This second > implementation uses JSON notation with "/" separator. I cannot satisfy > both libraries at the same time, and I agree with you last comment that > without a separator is misleading. > > If we need an additional library to link to SWUpdate, depends which new > services we need. Currently, I just needed a few functions to provide > the new path and as you have seen, I implemented myself. Of course, if > we need much more or we reuse in other parts of SWUpdate, it makes sense > to switch to a general library. > >> >> [1] https://likle.github.io/cwalk/ >> >> The code could append a relative reference to the current path or use an >> absolute reference as path. Afterwards it normalize the path and >> eliminate all '../', './' or '//'. An additional printf like function >> with a format string and arguments could simplify the path generation. >> > > I will take a look, thanks. But I think this can be done as optimization > and future enhancements and we do not need to switch now, if there are > no big issues with the current implementation. I would like to push a > release soon, and these enhancements can be done after release. > >> [snip] >> >>>>> diff --git a/corelib/parsing_library_libjson.c >>>>> b/corelib/parsing_library_libjson.c >>>>> index b1f5775..0322a4f 100644 >>>>> --- a/corelib/parsing_library_libjson.c >>>>> +++ b/corelib/parsing_library_libjson.c >>>>> @@ -7,6 +7,7 @@ >>>>> #include <stdio.h> >>>>> #include <stdlib.h> >>>>> #include <string.h> >>>>> +#include <stdbool.h> >>>>> #include <sys/types.h> >>>>> #include <unistd.h> >>>>> #include <fcntl.h> >>>>> @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, >>>>> const char *key) >>>>> : strndup(json_object_get_string(json_data), >>>>> MAX_URL_LENGTH); >>>>> } >>>>> +void *find_root_json(json_object *root, const char **nodes, >>>>> unsigned int depth) >>>>> +{ >>>>> + json_object *node; >>>>> + enum json_type type; >>>>> + char **tmp = NULL; >>>>> + const char *str; >>>>> + >>>>> + /* >>>>> + * check for deadlock links, block recursion >>>>> + */ >>>>> + if (!(--depth)) >>>>> + return false; >>>>> + >>>>> + node = find_json_recursive_node(root, nodes); >>>>> + >>>>> + if (node) { >>>>> + type = json_object_get_type(node); >>>>> + >>>>> + if (type == json_type_object || type == json_type_array) { >>>>> + str = get_field_string_json(node, "ref"); >>>>> + if (str) { >>>>> + if (!set_find_path(nodes, str, tmp)) >>>>> + return NULL; >>>>> + node = find_root_json(root, nodes, depth); >>>>> + free_string_array(tmp); >>>> >>>> It isn't clear why you need the tmp. Isn't it possible to remove this >>>> requirement for the set_find_path function? >>> >>> This comes from the choice to have an array of char**, and the caller is >>> the allocator. I guess it can be removed after switching to a list. >> >> I think in both cases you could free the unused strings in the function. > > I take a look at this point. It is also what I do not like, but I had > not found a better solution. > >> >> I really think a string path would be better because it is much more >> simpler to handle even if you have to parse it during every normalization. > > I think about, yes. C is not the best language to handle strings, and it > is easy to introduce bugs or memory leaks. > >> >>> >>>> >>>>> + } >>>>> + } >>>>> + } >>>>> + return node; >>>>> +} >>>>> + >>>>> +void *get_node_json(json_object *root, const char **nodes) >>>>> +{ >>>>> + return find_json_recursive_node(root, nodes); >>>>> +} >>>>> + >> >> [snip] I agree. This could be optimization with future enhancements. Best regards Stefan
diff --git a/corelib/parsing_library.c b/corelib/parsing_library.c index 00463a7..fd451ac 100644 --- a/corelib/parsing_library.c +++ b/corelib/parsing_library.c @@ -153,6 +153,39 @@ int exist_field_string(parsertype p, void *e, const char *path) return 0; } +void *find_root(parsertype p, void *root, const char **nodes) +{ + + switch (p) { + case LIBCFG_PARSER: + return find_root_libconfig((config_t *)root, nodes, MAX_LINKS_DEPTH); + + case JSON_PARSER: + return find_root_json((json_object *)root, nodes, MAX_LINKS_DEPTH); + default: + (void)root; + (void)nodes; + } + + return NULL; +} + +void *get_node(parsertype p, void *root, const char **nodes) +{ + + switch (p) { + case LIBCFG_PARSER: + return get_node_libconfig((config_t *)root, nodes); + case JSON_PARSER: + return get_node_json((json_object *)root, nodes); + default: + (void)root; + (void)nodes; + } + + return NULL; +} + void get_hash_value(parsertype p, void *elem, unsigned char *hash) { char hash_ascii[80]; @@ -165,52 +198,86 @@ void get_hash_value(parsertype p, void *elem, unsigned char *hash) bool set_find_path(const char **nodes, const char *newpath, char **tmp) { - unsigned int nleading; - char **iter, **paths; - unsigned int count = count_string_array(nodes); - unsigned int countpaths; - - if (!newpath) - return false; + char **paths; + unsigned int count; + char *saveptr; + char *token, *ref; + bool first = true; + int allocstr = 0; /* - * Check if we have to traverse back + * Include of files is not supported, + * each reference should start with '#' */ - for (nleading = 0; newpath[nleading] == '.'; nleading++); - - /* - * delimiter at the beginning indicates a relative path - * exactly as in Unix, that mean .. for the upper directory - * .. = parent - * .... = parent of parent - * The number of leading "." must be even, else - * it is a malformed path - */ - if (nleading % 2) + if (!newpath || (newpath[0] != '#') || (strlen(newpath) < 3)) return false; - nleading /= 2; - if ((count - nleading) <= 0) + ref = strdup(newpath); + if (!ref) { + ERROR("No memory: failed for %lu bytes", + strlen(newpath) + 1); return false; - - count -= nleading; - if (count > 0) count--; - - paths = string_split(newpath, '.'); + } /* - * check if there is enough space in nodes + * First find how many strings should be stored + * Each token is stored temporarily on the heap */ - countpaths = count_string_array((const char **)paths); - if (count + countpaths >= MAX_PARSED_NODES) + count = 0; + token = strtok_r(&ref[1], "/", &saveptr); + while (token) { + count++; + token = strtok_r(NULL, "/", &saveptr); + } + free(ref); /* string was changed, clean and copy again */ + + if (!count) + return false; + + paths = calloc(count + 1, sizeof(char*) * count); + if (!paths) { + ERROR("No memory: calloc failed for %lu bytes", + sizeof(char*) * count); + return false; + } + count = count_string_array(nodes); + ref = strdup(newpath); + if (!ref) { + ERROR("No memory: failed for %lu bytes", + strlen(newpath) + 1); + free(paths); return false; - if (!countpaths) - nodes[count++] = newpath; - else - for (iter = paths; *iter != NULL; iter++, count++) - nodes[count] = *iter; - nodes[count] = NULL; + } + + token = strtok_r(&ref[1], "/", &saveptr); + while (token) { + if (!strcmp(token, "..")) { + if (!count) { + free(ref); + free_string_array(paths); + return false; + } + count--; + } else if (strcmp(token, ".")) { + if (first) { + count = 0; + } + paths[allocstr] = strdup(token); + nodes[count++] = paths[allocstr++]; + paths[allocstr] = NULL; + nodes[count] = NULL; + if (count >= MAX_PARSED_NODES) { + ERROR("Big depth in link, giving up..."); + free_string_array(paths); + free(ref); + return false; + } + } + token = strtok_r(NULL, "/", &saveptr); + first = false; + } + free(ref); tmp = paths; return true; diff --git a/corelib/parsing_library_libconfig.c b/corelib/parsing_library_libconfig.c index 415116b..d3fc3c2 100644 --- a/corelib/parsing_library_libconfig.c +++ b/corelib/parsing_library_libconfig.c @@ -13,12 +13,34 @@ #include <errno.h> #include <sys/stat.h> #include <assert.h> +#include <stdbool.h> #include "generated/autoconf.h" #include "bsdqueue.h" #include "util.h" #include "swupdate.h" #include "parselib.h" +static void path_libconfig(const char **nodes, char *root, unsigned int rootsize) +{ + const char **node; + int nbytes, left; + char *buf; + const char *concat; + bool first=true; + + root[0] = '\0'; + + for (node = nodes, buf = root, left = rootsize; *node != NULL; node++) { + concat = first ? "" : "."; + nbytes = snprintf(buf, left, "%s%s", concat, *node); + buf += nbytes; + left -= nbytes; + first = false; + if (left ==0) + break; + } +} + void get_value_libconfig(const config_setting_t *e, void *dest) { int type = config_setting_type(e); @@ -108,3 +130,52 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path) return NULL; } + +void *get_node_libconfig(config_t *cfg, const char **nodes) +{ + config_setting_t *setting; + char root[1024]; + + path_libconfig(nodes, root, sizeof(root)); + setting = config_lookup(cfg, root); + if (setting) + return setting; + + return NULL; +} + +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned int depth) +{ + config_setting_t *elem; + char root[1024]; + const char *ref; + char **tmp = NULL; + + /* + * check for deadlock links, block recursion + */ + if (!(--depth)) + return NULL; + + path_libconfig(nodes, root, sizeof(root)); + + /* + * If this is root node for the device, + * it is a group and lenght is not 0. + * If it is a link, follow it + */ + elem = config_lookup(cfg, root); + + if (elem && config_setting_is_group(elem) == CONFIG_TRUE) { + ref = get_field_string_libconfig(elem, "ref"); + if (ref) { + if (!set_find_path(nodes, ref, tmp)) + return NULL; + elem = find_root_libconfig(cfg, nodes, depth); + free_string_array(tmp); + } + } + + return elem; + +} diff --git a/corelib/parsing_library_libjson.c b/corelib/parsing_library_libjson.c index b1f5775..0322a4f 100644 --- a/corelib/parsing_library_libjson.c +++ b/corelib/parsing_library_libjson.c @@ -7,6 +7,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <stdbool.h> #include <sys/types.h> #include <unistd.h> #include <fcntl.h> @@ -179,4 +180,40 @@ char *json_get_data_url(json_object *json_root, const char *key) : strndup(json_object_get_string(json_data), MAX_URL_LENGTH); } +void *find_root_json(json_object *root, const char **nodes, unsigned int depth) +{ + json_object *node; + enum json_type type; + char **tmp = NULL; + const char *str; + + /* + * check for deadlock links, block recursion + */ + if (!(--depth)) + return false; + + node = find_json_recursive_node(root, nodes); + + if (node) { + type = json_object_get_type(node); + + if (type == json_type_object || type == json_type_array) { + str = get_field_string_json(node, "ref"); + if (str) { + if (!set_find_path(nodes, str, tmp)) + return NULL; + node = find_root_json(root, nodes, depth); + free_string_array(tmp); + } + } + } + return node; +} + +void *get_node_json(json_object *root, const char **nodes) +{ + return find_json_recursive_node(root, nodes); +} + diff --git a/include/parselib.h b/include/parselib.h index 2fca3b5..5cbc255 100644 --- a/include/parselib.h +++ b/include/parselib.h @@ -39,6 +39,8 @@ 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); +void *find_root_libconfig(config_t *cfg, const char **nodes, unsigned int depth); +void *get_node_libconfig(config_t *cfg, const char **nodes); #else #define config_setting_get_elem(a,b) (NULL) @@ -49,6 +51,8 @@ const char *get_field_string_libconfig(config_setting_t *e, const char *path); #define get_child_libconfig(e, name) (NULL) #define iterate_field_libconfig(e, cb, data) { } #define get_field_cfg(e, path, dest) +#define find_root_libconfig(cfg, nodes, depth) (NULL) +#define get_node_libconfig(cfg, nodes) (NULL) #endif #ifdef CONFIG_JSON @@ -65,6 +69,8 @@ const char *json_get_value(struct json_object *json_root, const char *key); json_object *json_get_path_key(json_object *json_root, const char **json_path); char *json_get_data_url(json_object *json_root, const char *key); +void *find_root_json(json_object *root, const char **nodes, unsigned int depth); +void *get_node_json(json_object *root, const char **nodes); #else #define find_node_json(a, b, c) (NULL) @@ -75,6 +81,8 @@ char *json_get_data_url(json_object *json_root, const char *key); #define json_object_object_get_ex(a,b,c) (0) #define json_object_array_get_idx(a, b) (0) #define json_object_array_length(a) (0) +#define find_root_json(root, nodes, depth) (NULL) +#define get_node_json(root, nodes) (NULL) #endif typedef int (*settings_callback)(void *elem, void *data); @@ -90,7 +98,7 @@ 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); void check_field_string(const char *src, char *dst, const size_t max_len); -bool find_root(parsertype p, void *root, const char **nodes); +void *find_root(parsertype p, void *root, const char **nodes); void *get_node(parsertype p, void *root, const char **nodes); bool set_find_path(const char **nodes, const char *newpath, char **tmp); diff --git a/parser/parser.c b/parser/parser.c index 19181d1..bc89b9d 100644 --- a/parser/parser.c +++ b/parser/parser.c @@ -11,6 +11,7 @@ #include <string.h> #include <sys/types.h> #include <unistd.h> +#include <stdbool.h> #include <fcntl.h> #include <errno.h> #include <sys/stat.h> @@ -29,115 +30,125 @@ #define NODEROOT (!strlen(CONFIG_PARSERROOT) ? \ "software" : CONFIG_PARSERROOT) -#ifdef CONFIG_LIBCONFIG -static config_setting_t *find_node_libconfig(config_t *cfg, - const char *field, struct swupdate_cfg *swcfg) +#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) + +static bool path_append(const char **nodes, const char *field) { - config_setting_t *setting; - struct hw_type *hardware; + unsigned int count = 0; - char node[1024]; + count = count_string_array(nodes); - if (!field) - return NULL; + if (count >= MAX_PARSED_NODES) + return false; - hardware = &swcfg->hw; + nodes[count++] = field; + nodes[count] = NULL; - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { - /* Try with both software set and board name */ - if (strlen(hardware->boardname)) { - snprintf(node, sizeof(node), "%s.%s.%s.%s.%s", - NODEROOT, - hardware->boardname, - swcfg->software_set, - swcfg->running_mode, - field); - setting = config_lookup(cfg, node); - if (setting) - return setting; - } - /* still here, try with software set and mode */ - snprintf(node, sizeof(node), "%s.%s.%s.%s", - NODEROOT, - swcfg->software_set, - swcfg->running_mode, - field); - setting = config_lookup(cfg, node); - if (setting) - return setting; - - } - - /* Try with board name */ - if (strlen(hardware->boardname)) { - snprintf(node, sizeof(node), "%s.%s.%s", - NODEROOT, - hardware->boardname, - field); - setting = config_lookup(cfg, node); - if (setting) - return setting; - } - /* Fall back without board entry */ - snprintf(node, sizeof(node), "%s.%s", - NODEROOT, - field); - return config_lookup(cfg, node); + return true; } -#endif - -#ifdef CONFIG_JSON -static json_object *find_node_json(json_object *root, const char *node, +static void *find_node(parsertype p, void *root, const char *field, struct swupdate_cfg *swcfg) { - json_object *jnode = NULL; - const char *simple_nodes[] = {NODEROOT, node, NULL}; + struct hw_type *hardware; + const char **nodes; + int i; + + if (!field) + return NULL; hardware = &swcfg->hw; - if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { - if (strlen(hardware->boardname)) { - const char *nodes[] = {NODEROOT, hardware->boardname, - swcfg->software_set, swcfg->running_mode, - node, NULL}; - jnode = find_json_recursive_node(root, nodes); - if (jnode) - return jnode; - } else { - const char *nodes[] = {NODEROOT, swcfg->software_set, - swcfg->running_mode, node, NULL}; - jnode = find_json_recursive_node(root, nodes); - if (jnode) - return jnode; + nodes = (const char **)calloc(MAX_PARSED_NODES, sizeof(*nodes)); + + for (i = 0; i < 4; i++) { + nodes[0] = NULL; + switch(i) { + case 0: + if (strlen(swcfg->running_mode) && strlen(swcfg->software_set) && + strlen(hardware->boardname)) { + nodes[0] = NODEROOT; + nodes[1] = hardware->boardname; + nodes[2] = swcfg->software_set; + nodes[3] = swcfg->running_mode; + nodes[4] = NULL; + } + break; + case 1: + /* try with software set and mode */ + if (strlen(swcfg->running_mode) && strlen(swcfg->software_set)) { + nodes[0] = NODEROOT; + nodes[1] = swcfg->software_set; + nodes[2] = swcfg->running_mode; + nodes[3] = NULL; + } + break; + case 2: + /* Try with board name */ + if (strlen(hardware->boardname)) { + nodes[0] = NODEROOT; + nodes[1] = hardware->boardname; + nodes[2] = NULL; + } + break; + case 3: + /* Fall back without board entry */ + nodes[0] = NODEROOT; + nodes[1] = NULL; + break; } - } - if (strlen(hardware->boardname)) { - const char *nodes[] = {NODEROOT, hardware->boardname, node, - NULL}; - jnode = find_json_recursive_node(root, nodes); - if (jnode) - return jnode; + /* + * If conditions are not set, + * skip to the next option + */ + if (!nodes[0]) + continue; + + /* + * The first find_root() search for + * the root element from board, selection + * The second one starts from root and follow the tree + * to search for element + */ + if (find_root(p, root, nodes)) { + void *node = NULL; + if (!path_append(nodes, field)) + return NULL; + node = find_root(p, root, nodes); + + if (node) { + free(nodes); + return node; + } + } } - return find_json_recursive_node(root, simple_nodes); + free(nodes); + + return NULL; } -#endif -#if defined(CONFIG_LIBCONFIG) || defined(CONFIG_JSON) -static void *find_node(parsertype p, void *root, const char *node, - struct swupdate_cfg *swcfg) +static bool get_common_fields(parsertype p, void *cfg, struct swupdate_cfg *swcfg) { - switch (p) { - case LIBCFG_PARSER: - return find_node_libconfig((config_t *)root, node, swcfg); - case JSON_PARSER: - return find_node_json((json_object *)root, node, swcfg); + + void *setting; + + if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { + ERROR("Missing version in configuration file"); + return false; } + + GET_FIELD_STRING(p, setting, NULL, swcfg->version); + TRACE("Version %s", swcfg->version); - return NULL; + if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { + GET_FIELD_STRING(p, setting, NULL, swcfg->description); + TRACE("Description %s", swcfg->description); + } + + return true; } static void add_properties_cb(const char *name, const char *value, void *data) @@ -672,11 +683,8 @@ static int parser(parsertype p, void *cfg, struct swupdate_cfg *swcfg) int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) { config_t cfg; - const char *str; - char node[128]; parsertype p = LIBCFG_PARSER; int ret; - config_setting_t *setting; memset(&cfg, 0, sizeof(cfg)); config_init(&cfg); @@ -694,24 +702,8 @@ int parse_cfg (struct swupdate_cfg *swcfg, const char *filename) return -1; } - if((setting = find_node(p, &cfg, "version", swcfg)) == NULL) { - ERROR("Missing version in configuration file"); + if (!get_common_fields(p, &cfg, swcfg)) return -1; - } else { - GET_FIELD_STRING(p, setting, NULL, swcfg->version); - TRACE("Version %s", swcfg->version); - } - - if((setting = find_node(p, &cfg, "description", swcfg)) != NULL) { - GET_FIELD_STRING(p, setting, NULL, swcfg->description); - TRACE("Description %s", swcfg->description); - } - - snprintf(node, sizeof(node), "%s.embedded-script", - NODEROOT); - if (config_lookup_string(&cfg, node, &str)) { - TRACE("Found Lua Software:\n%s", str); - } ret = parser(p, &cfg, swcfg); @@ -734,7 +726,7 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) struct stat stbuf; unsigned int size; char *string; - json_object *cfg, *setting; + json_object *cfg; parsertype p = JSON_PARSER; /* Read the file. If there is an error, report it and exit. */ @@ -764,18 +756,9 @@ int parse_json(struct swupdate_cfg *swcfg, const char *filename) return -1; } - if((setting = find_node(p, cfg, "version", swcfg)) == NULL) { - ERROR("Missing version in configuration file"); + if (!get_common_fields(p, cfg, swcfg)) { free(string); return -1; - } else { - GET_FIELD_STRING(p, setting, NULL, swcfg->version); - TRACE("Version %s", swcfg->version); - } - - if((setting = find_node(p, cfg, "description", swcfg)) != NULL) { - GET_FIELD_STRING(p, setting, NULL, swcfg->description); - TRACE("Description %s", swcfg->description); } ret = parser(p, cfg, swcfg);
There are a lot of use cases when special configurations are needed. Current sw-description requires that each entry is full described and does not allow to factorize some parts. This means that in most cases entries are duplicated multiple times even if changes are minimal between sections. This patch introduce links for the root node. If the node is a string instead of an object, it is interpreted as new link to be follow. The parser tries to follow the links recursively (a maximum number of 10 links is introduce to avoid deadlocks). The patch factorizes how to call find_node() for the specific parser. Instead of using two mechanism (a formatted sting and an array of strings) for libconfig and json, use an array of strings for both. Signed-off-by: Stefano Babic <sbabic@denx.de> --- Changes in v2: None corelib/parsing_library.c | 137 ++++++++++++----- corelib/parsing_library_libconfig.c | 71 +++++++++ corelib/parsing_library_libjson.c | 37 +++++ include/parselib.h | 10 +- parser/parser.c | 219 +++++++++++++--------------- 5 files changed, 320 insertions(+), 154 deletions(-)