[v2,2/5] corelib: Rework dictionary to support multiple values per key

Message ID 1515769128-29657-2-git-send-email-stefan@herbrechtsmeier.net
State Changes Requested
Headers show
Series
  • [v2,1/5] dict: Rename dictionary struct and its key to distinguish it from simple lists
Related show

Commit Message

Stefan Herbrechtsmeier Jan. 12, 2018, 2:58 p.m.
From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

Changes in v2: None

 bootloader/grub.c             |  20 +++----
 corelib/installer.c           |   8 +--
 corelib/swupdate_dict.c       | 124 ++++++++++++++++++++++++++++++++----------
 handlers/swuforward_handler.c |   6 +-
 include/swupdate_dict.h       |  16 +++++-
 parser/parser.c               |   2 +-
 suricatta/server_hawkbit.c    |  14 +++--
 7 files changed, 138 insertions(+), 52 deletions(-)

Comments

Stefan Herbrechtsmeier Jan. 15, 2018, 7:48 a.m. | #1
Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: stefan@herbrechtsmeier.net [mailto:stefan@herbrechtsmeier.net]
> Gesendet: Freitag, 12. Januar 2018 15:59
> An: swupdate@googlegroups.com
> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
> <Stefan.Herbrechtsmeier@weidmueller.com>
> Betreff: [PATCH v2 2/5] corelib: Rework dictionary to support multiple values
> per key
>
> From: Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier@weidmueller.com>
>
> Signed-off-by: Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier@weidmueller.com>
> ---
>
> Changes in v2: None
>
>  bootloader/grub.c             |  20 +++----
>  corelib/installer.c           |   8 +--
>  corelib/swupdate_dict.c       | 124 ++++++++++++++++++++++++++++++++-
> ---------
>  handlers/swuforward_handler.c |   6 +-
>  include/swupdate_dict.h       |  16 +++++-
>  parser/parser.c               |   2 +-
>  suricatta/server_hawkbit.c    |  14 +++--
>  7 files changed, 138 insertions(+), 52 deletions(-)

[snip]


> diff --git a/handlers/swuforward_handler.c
> b/handlers/swuforward_handler.c index 54aef4b..72d6cff 100644
> --- a/handlers/swuforward_handler.c
> +++ b/handlers/swuforward_handler.c
> @@ -338,8 +338,10 @@ static int install_remote_swu(struct img_type *img,
>        */
>       LIST_FOREACH(url, &img->properties, next) {
>               char curlheader[SWUPDATE_GENERAL_STRING_SIZE +
> strlen(CUSTOM_HEADER)];
> +             char *key = dict_entry_get_key(url);
> +             char *value = dict_entry_get_value(url);
>
> -             if (!url->key || !url->value || strcmp(url->key, "url"))
> +             if (!key || !value || strcmp(key, "url"))
>                       continue;
>
>               conn = (struct curlconn *)calloc(1, sizeof(struct curlconn));
> @@ -352,7 +354,7 @@ static int install_remote_swu(struct img_type *img,
>               headerlist = NULL;
>
>               conn->curl_handle = curl_easy_init();
> -             conn->url = url->value;
> +             conn->url = value;
>
>               if (!conn->curl_handle) {
>                       /* something very bad, it should never happen */ diff

I forget to update the swuforward_handler to iterate over the list of "url" values. I will do it in the next version after I get some feedback from you.

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
Stefano Babic Jan. 15, 2018, 8:51 a.m. | #2
Hi Stefan,

On 15/01/2018 08:48, Stefan.Herbrechtsmeier@weidmueller.com wrote:
> Hi Stefano,
> 
>> -----Ursprüngliche Nachricht-----
>> Von: stefan@herbrechtsmeier.net [mailto:stefan@herbrechtsmeier.net]
>> Gesendet: Freitag, 12. Januar 2018 15:59
>> An: swupdate@googlegroups.com
>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan
>> <Stefan.Herbrechtsmeier@weidmueller.com>
>> Betreff: [PATCH v2 2/5] corelib: Rework dictionary to support multiple values
>> per key
>>
>> From: Stefan Herbrechtsmeier
>> <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Signed-off-by: Stefan Herbrechtsmeier
>> <stefan.herbrechtsmeier@weidmueller.com>
>> ---
>>
>> Changes in v2: None
>>
>>  bootloader/grub.c             |  20 +++----
>>  corelib/installer.c           |   8 +--
>>  corelib/swupdate_dict.c       | 124 ++++++++++++++++++++++++++++++++-
>> ---------
>>  handlers/swuforward_handler.c |   6 +-
>>  include/swupdate_dict.h       |  16 +++++-
>>  parser/parser.c               |   2 +-
>>  suricatta/server_hawkbit.c    |  14 +++--
>>  7 files changed, 138 insertions(+), 52 deletions(-)
> 
> [snip]
> 
> 
>> diff --git a/handlers/swuforward_handler.c
>> b/handlers/swuforward_handler.c index 54aef4b..72d6cff 100644
>> --- a/handlers/swuforward_handler.c
>> +++ b/handlers/swuforward_handler.c
>> @@ -338,8 +338,10 @@ static int install_remote_swu(struct img_type *img,
>>        */
>>       LIST_FOREACH(url, &img->properties, next) {
>>               char curlheader[SWUPDATE_GENERAL_STRING_SIZE +
>> strlen(CUSTOM_HEADER)];
>> +             char *key = dict_entry_get_key(url);
>> +             char *value = dict_entry_get_value(url);
>>
>> -             if (!url->key || !url->value || strcmp(url->key, "url"))
>> +             if (!key || !value || strcmp(key, "url"))
>>                       continue;
>>
>>               conn = (struct curlconn *)calloc(1, sizeof(struct curlconn));
>> @@ -352,7 +354,7 @@ static int install_remote_swu(struct img_type *img,
>>               headerlist = NULL;
>>
>>               conn->curl_handle = curl_easy_init();
>> -             conn->url = url->value;
>> +             conn->url = value;
>>
>>               if (!conn->curl_handle) {
>>                       /* something very bad, it should never happen */ diff
> 
> I forget to update the swuforward_handler to iterate over the list of "url" values. I will do it in the next version after I get some feedback from you.
> 

Yes, I have seen it - it takes just the last element in the array. I
have tested yesterday and I have not found (apart this) big issues.

I have fixed the issue ( { } instead of (0) when LUA is not activated),
there is an additional warning using with_lua_nohandlers_defconfig:

corelib/parsing_library.c: In function ‘iterate_field’:
corelib/parsing_library.c:77:60: warning: unused parameter ‘cb’
[-Wunused-parameter]
 void iterate_field(parsertype p, void *e, iterate_callback cb, void *data)

This should be fixed, too.

Apart code in swuforward_handler.c, we need to update the documentation,
too (doc/source/handlers.rst).

Else patch is fine :-)

Best regards,
Stefano

Patch

diff --git a/bootloader/grub.c b/bootloader/grub.c
index 93682f0..966498f 100644
--- a/bootloader/grub.c
+++ b/bootloader/grub.c
@@ -165,8 +165,10 @@  static inline void grubenv_update_size(struct grubenv_t *grubenv)
 
 	/* lengths of strings + '=' and '\n' characters */
 	LIST_FOREACH(grubvar, &grubenv->vars, next) {
-		size = size + strlen(grubvar->key) +
-						strlen(grubvar->value) + 2;
+		char *key = dict_entry_get_key(grubvar);
+		char *value = dict_entry_get_value(grubvar);
+
+		size = size + strlen(key) + strlen(value) + 2;
 	}
 	size += strlen(GRUBENV_HEADER);
 	grubenv->size = size;
@@ -206,10 +208,12 @@  static int grubenv_write(struct grubenv_t *grubenv)
 	strncpy(buf, GRUBENV_HEADER, strlen(GRUBENV_HEADER) + 1);
 
 	LIST_FOREACH(grubvar, &grubenv->vars, next) {
-		llen = strlen(grubvar->key) + strlen(grubvar->value) + 2;
+		char *key = dict_entry_get_key(grubvar);
+		char *value = dict_entry_get_value(grubvar);
+
+		llen = strlen(key) + strlen(value) + 2;
 		/* +1 for null termination */
-		snprintf(line, llen + 1, "%s=%s\n", grubvar->key,
-						grubvar->value);
+		snprintf(line, llen + 1, "%s=%s\n", key, value);
 		strncat(buf, line, llen);
 	}
 
@@ -249,11 +253,7 @@  cleanup:
  * allocation */
 static inline void grubenv_close(struct grubenv_t *grubenv)
 {
-	struct dict_entry *grubvar;
-
-	LIST_FOREACH(grubvar, &grubenv->vars, next) {
-		dict_remove(&grubenv->vars, grubvar->key);
-	}
+	dict_drop_db(&grubenv->vars);
 }
 
 /* I feel that '#' and '=' characters should be forbidden. Although it's not
diff --git a/corelib/installer.c b/corelib/installer.c
index e25c30c..7da49b2 100644
--- a/corelib/installer.c
+++ b/corelib/installer.c
@@ -218,11 +218,11 @@  static int prepare_boot_script(struct swupdate_cfg *cfg, const char *script)
 		return -1;
 
 	LIST_FOREACH(bootvar, &cfg->bootloader, next) {
-		if (!bootvar->key || !bootvar->value)
+		char *key = dict_entry_get_key(bootvar);
+		char *value = dict_entry_get_value(bootvar);
+		if (!key || !value)
 			continue;
-		snprintf(buf, sizeof(buf), "%s %s\n",
-			bootvar->key,
-			bootvar->value);
+		snprintf(buf, sizeof(buf), "%s %s\n", key, value);
 		if (write(fd, buf, strlen(buf)) != (ssize_t)strlen(buf)) {
 			  TRACE("Error saving temporary file");
 			  ret = -1;
diff --git a/corelib/swupdate_dict.c b/corelib/swupdate_dict.c
index 352c13a..46137a0 100644
--- a/corelib/swupdate_dict.c
+++ b/corelib/swupdate_dict.c
@@ -2,6 +2,9 @@ 
  * (C) Copyright 2016
  * Stefano Babic, DENX Software Engineering, sbabic@denx.de.
  *
+ * Copyright (C) 2018 Weidmüller Interface GmbH & Co. KG
+ * Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+ *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
  * published by the Free Software Foundation; either version 2 of
@@ -31,6 +34,51 @@ 
 #include "util.h"
 #include "swupdate_dict.h"
 
+static int insert_list_elem(struct dict_list *list, char *value)
+{
+	struct dict_list_elem *elem = (struct dict_list_elem *)malloc(sizeof(*elem));
+
+	if (!elem)
+		return -ENOMEM;
+
+	memset(elem, 0, sizeof(*elem));
+	elem->value = strdup(value);
+
+	LIST_INSERT_HEAD(list, elem, next);
+
+	return 0;
+}
+
+static void remove_list_elem(struct dict_list_elem *elem)
+{
+	LIST_REMOVE(elem, next);
+	free(elem->value);
+	free(elem);
+}
+
+static void remove_list(struct dict_list *list)
+{
+	struct dict_list_elem *elem;
+
+	LIST_FOREACH(elem, list, next) {
+		remove_list_elem(elem);
+	}
+}
+
+static struct dict_entry *insert_entry(struct dict *dictionary, char *key)
+{
+	struct dict_entry *entry = (struct dict_entry *)malloc(sizeof(*entry));
+	if (!entry)
+		return NULL;
+
+	memset(entry, 0, sizeof(*entry));
+	entry->key = strdup(key);
+
+	LIST_INSERT_HEAD(dictionary, entry, next);
+
+	return entry;
+}
+
 static struct dict_entry *get_entry(struct dict *dictionary, char *key)
 {
 	struct dict_entry *entry;
@@ -43,20 +91,38 @@  static struct dict_entry *get_entry(struct dict *dictionary, char *key)
 	return NULL;
 }
 
-int dict_insert_entry(struct dict *dictionary, char *key, char *value)
+static void remove_entry(struct dict_entry *entry)
 {
-	struct dict_entry *entry = (struct dict_entry *)malloc(sizeof(*entry));
+	LIST_REMOVE(entry, next);
+	free(entry->key);
+	remove_list(&entry->list);
+	free(entry);
+}
 
+char *dict_entry_get_key(struct dict_entry *entry)
+{
 	if (!entry)
-		return -ENOMEM;
+		return NULL;
 
-	memset(entry, 0, sizeof(*entry));
-	entry->key = strdup(key);
-	entry->value = strdup(value);
+	return entry->key;
+}
 
-	LIST_INSERT_HEAD(dictionary, entry, next);
+char *dict_entry_get_value(struct dict_entry *entry)
+{
+	if (!entry || !LIST_FIRST(&entry->list))
+		return NULL;
 
-	return 0;
+	return LIST_FIRST(&entry->list)->value;
+}
+
+struct dict_list *dict_get_list(struct dict *dictionary, char *key)
+{
+	struct dict_entry *entry = get_entry(dictionary, key);
+
+	if (!entry)
+		return NULL;
+
+	return &entry->list;
 }
 
 char *dict_get_value(struct dict *dictionary, char *key)
@@ -66,49 +132,51 @@  char *dict_get_value(struct dict *dictionary, char *key)
 	if (!entry)
 		return NULL;
 
-	return entry->value;
+	return dict_entry_get_value(entry);
 }
 
-int dict_set_value(struct dict *dictionary, char *key, char *value)
+int dict_insert_value(struct dict *dictionary, char *key, char *value)
 {
 	struct dict_entry *entry = get_entry(dictionary, key);
 
-	/*
-	 * Set to new value if key is already in
-	 * dictionary
-	 */
-	if (entry) {
-		LIST_REMOVE(entry, next);
-		free(entry);
+	if (!entry) {
+		entry = insert_entry(dictionary, key);
+		if (!entry)
+			return -ENOMEM;
 	}
 
-	return dict_insert_entry(dictionary, key, value);
+	return insert_list_elem(&entry->list, value);
 }
 
-void dict_remove_entry(struct dict_entry *entry)
+int dict_set_value(struct dict *dictionary, char *key, char *value)
 {
-	LIST_REMOVE(entry, next);
-	free(entry->key);
-	free(entry->value);
-	free(entry);
+	struct dict_entry *entry = get_entry(dictionary, key);
+
+	if (entry)
+		remove_entry(entry);
+
+	entry = insert_entry(dictionary, key);
+	if (!entry)
+		return -ENOMEM;
+
+	return insert_list_elem(&entry->list, value);
 }
 
 void dict_remove(struct dict *dictionary, char *key)
 {
-
 	struct dict_entry *entry = get_entry(dictionary, key);
 
 	if (!entry)
 		return;
 
-	dict_remove_entry(entry);
+	remove_entry(entry);
 }
 
 void dict_drop_db(struct dict *dictionary)
 {
-	struct dict_entry *var;
+	struct dict_entry *entry;
 
-	LIST_FOREACH(var, dictionary, next) {
-		dict_remove_entry(var);
+	LIST_FOREACH(entry, dictionary, next) {
+		remove_entry(entry);
 	}
 }
diff --git a/handlers/swuforward_handler.c b/handlers/swuforward_handler.c
index 54aef4b..72d6cff 100644
--- a/handlers/swuforward_handler.c
+++ b/handlers/swuforward_handler.c
@@ -338,8 +338,10 @@  static int install_remote_swu(struct img_type *img,
 	 */
 	LIST_FOREACH(url, &img->properties, next) {
 		char curlheader[SWUPDATE_GENERAL_STRING_SIZE + strlen(CUSTOM_HEADER)];
+		char *key = dict_entry_get_key(url);
+		char *value = dict_entry_get_value(url);
 
-		if (!url->key || !url->value || strcmp(url->key, "url"))
+		if (!key || !value || strcmp(key, "url"))
 			continue;
 
 		conn = (struct curlconn *)calloc(1, sizeof(struct curlconn));
@@ -352,7 +354,7 @@  static int install_remote_swu(struct img_type *img,
 		headerlist = NULL;
 
 		conn->curl_handle = curl_easy_init();
-		conn->url = url->value;
+		conn->url = value;
 
 		if (!conn->curl_handle) {
 			/* something very bad, it should never happen */
diff --git a/include/swupdate_dict.h b/include/swupdate_dict.h
index f8b696f..cd6b80c 100644
--- a/include/swupdate_dict.h
+++ b/include/swupdate_dict.h
@@ -23,19 +23,29 @@ 
 
 #include <bsdqueue.h>
 
+struct dict_list_elem {
+	char *value;
+	LIST_ENTRY(dict_list_elem) next;
+};
+
+LIST_HEAD(dict_list, dict_list_elem);
+
 struct dict_entry {
 	char *key;
-	char *value;
+	struct dict_list list;
 	LIST_ENTRY(dict_entry) next;
 };
 
 LIST_HEAD(dict, dict_entry);
 
+char *dict_entry_get_key(struct dict_entry *entry);
+char *dict_entry_get_value(struct dict_entry *entry);
+
+struct dict_list *dict_get_list(struct dict *dictionary, char *key);
 char *dict_get_value(struct dict *dictionary, char *key);
 int dict_set_value(struct dict *dictionary, char *key, char *value);
-int dict_insert_entry(struct dict *dictionary, char *key, char *value);
+int dict_insert_value(struct dict *dictionary, char *key, char *value);
 void dict_remove(struct dict *dictionary, char *key);
-void dict_remove_entry(struct dict_entry *entry);
 void dict_drop_db(struct dict *dictionary);
 
 #endif
diff --git a/parser/parser.c b/parser/parser.c
index 3a1d6a5..3ecc2da 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -174,7 +174,7 @@  static void add_properties(parsertype p, void *node, struct img_type *image)
 				key,
 				value
 			);
-			if (dict_insert_entry(&image->properties, key, value))
+			if (dict_insert_value(&image->properties, key, value))
 				ERROR("Property not stored, skipping...");
 
 		}
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index 51ed32f..b899715 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -1348,7 +1348,10 @@  int get_target_data_length(void)
 	struct dict_entry *entry;
 
 	LIST_FOREACH(entry, &server_hawkbit.configdata, next) {
-		len += strlen(entry->key) + strlen(entry->value) + strlen (" : ") + 6;
+		char *key = dict_entry_get_key(entry);
+		char *value = dict_entry_get_value(entry);
+
+		len += strlen(key) + strlen(value) + strlen (" : ") + 6;
 	}
 
 	return len;
@@ -1379,17 +1382,20 @@  server_op_res_t server_send_target_data(void)
 
 	char *keyvalue = NULL;
 	LIST_FOREACH(entry, &server_hawkbit.configdata, next) {
+		char *key = dict_entry_get_key(entry);
+		char *value = dict_entry_get_value(entry);
+
 		if (ENOMEM_ASPRINTF ==
 		    asprintf(&keyvalue, config_data,
 				((first) ? ' ' : ','),
-				entry->key,
-				entry->value)) {
+				key,
+				value)) {
 			ERROR("hawkBit server reply cannot be sent because of OOM.\n");
 			result = SERVER_EINIT;
 			goto cleanup;
 		}
 		first = false;
-		TRACE("KEYVALUE=%s %s %s", keyvalue, entry->key, entry->value);
+		TRACE("KEYVALUE=%s %s %s", keyvalue, key, value);
 		strcat(configData, keyvalue);
 		free(keyvalue);