diff mbox series

[2/3] server_hawkbit: make ipc_lock non-recursive

Message ID 20211122031518.2201903-2-dominique.martinet@atmark-techno.com
State Accepted
Headers show
Series [1/3] server_hawkbit: fix typo in function name s/hakwbit/hawkbit/ | expand

Commit Message

Dominique MARTINET Nov. 22, 2021, 3:15 a.m. UTC
musl does not have PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, instead one
is expected to call pthread_mutex_init with PTHREAD_MUTEX_RECURSIVE before
using the lock.

Unfortunately, this part of the code does not allow for easy init before use:
start_suricatta() will first start the ipc thread, then call server_start,
but we do not have any guarantee that the ipc thread will not call server_ipc
before server_start has had a chance to initialize the mutex.

Thanksfully we apparently only need the recursivity here for a tiny portion
of the code: get_target_data_length() can be called once with lock and once
without lock, so just adding a toggle to get the lock is enough.

Signed-off-by: Dominique Martinet <dominique.martinet@atmark-techno.com>
Cc: Roland Gaudig <roland.gaudig@weidmueller.com>
---

Roland, could you please comment if that sounds good for you?

The only alternative I see would be adding a server_init call before
server_start, but I'd rather not change any API if I don't have to.

I've tested it on our boards and there doesn't seem to be any recursive
locking happening except the one I fixed (at least I didn't experience
any deadlock), but it's possible I missed some in code we don't use

 suricatta/server_hawkbit.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
diff mbox series

Patch

diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index dbce9a35808c..86f050dc2c86 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -58,7 +58,7 @@  static struct option long_options[] = {
 
 static unsigned short mandatory_argument_count = 0;
 static pthread_mutex_t notifylock = PTHREAD_MUTEX_INITIALIZER;
-static pthread_mutex_t ipc_lock = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+static pthread_mutex_t ipc_lock = PTHREAD_MUTEX_INITIALIZER;
 
 /*
  * See hawkBit's API for an explanation
@@ -109,7 +109,7 @@  server_send_deployment_reply(channel_t *channel,
 			     const int job_cnt_cur, const char *finished,
 			     const char *execution_status, int numdetails, const char *details[]);
 server_op_res_t server_send_cancel_reply(channel_t *channel, const int action_id);
-static int get_target_data_length(void);
+static int get_target_data_length(bool locked);
 
 server_hawkbit_t server_hawkbit = {.url = NULL,
 				   .polling_interval = CHANNEL_DEFAULT_POLLING_INTERVAL,
@@ -526,7 +526,7 @@  server_op_res_t server_set_config_data(json_object *json_root)
 		if (server_hawkbit.configData_url)
 			free(server_hawkbit.configData_url);
 		server_hawkbit.configData_url = tmp;
-		server_hawkbit.has_to_send_configData = (get_target_data_length() > 0) ? true : false;
+		server_hawkbit.has_to_send_configData = (get_target_data_length(true) > 0) ? true : false;
 		TRACE("ConfigData: %s", server_hawkbit.configData_url);
 		pthread_mutex_unlock(&ipc_lock);
 	}
@@ -1471,19 +1471,21 @@  server_op_res_t server_install_update(void)
 	return result;
 }
 
-int get_target_data_length(void)
+int get_target_data_length(bool locked)
 {
 	int len = 0;
 	struct dict_entry *entry;
 
-	pthread_mutex_lock(&ipc_lock);
+	if (!locked)
+		pthread_mutex_lock(&ipc_lock);
 	LIST_FOREACH(entry, &server_hawkbit.configdata, next) {
 		char *key = dict_entry_get_key(entry);
 		char *value = dict_entry_get_value(entry);
 
 		len += strlen(key) + strlen(value) + strlen (" : ") + 6;
 	}
-	pthread_mutex_unlock(&ipc_lock);
+	if (!locked)
+		pthread_mutex_unlock(&ipc_lock);
 
 	return len;
 }
@@ -1499,7 +1501,7 @@  server_op_res_t server_send_target_data(void)
 	char *url = NULL;
 
 	assert(channel != NULL);
-	len = get_target_data_length();
+	len = get_target_data_length(false);
 
 	if (!len) {
 		server_hawkbit.has_to_send_configData = false;