[PATCH 0/2] alfred: notifications: random fixes

[PATCH 0/2] alfred: notifications: random fixes

[Sven Eckelmann]

Similar to the batctl fixes, I have now compiled a list of potential fixes
for alfred. Instead of posting everything at once, I will be posted in
smaller chunks. They are just various (mostly minor) problems which should
be tackled at some point.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Sven Eckelmann (2):
      alfred: Use _exit() in the update command child process
      alfred: Announce changed data only after it was stored

 recv.c      | 41 +++++++++++++++++++++++++----------------
 server.c    |  2 +-
 unix_sock.c | 43 ++++++++++++++++++++++++++++---------------
 3 files changed, 54 insertions(+), 32 deletions(-)
---
base-commit: 462a0cf40615f62e0912c36d72a75accbb26f5ee
change-id: 20260730-bugfixes-notification-40868d6d799c

Best regards,
--  
Sven Eckelmann <sven@narfation.org>

[PATCH 1/2] alfred: Use _exit() in the update command child process

[Sven Eckelmann]

execute_update_command() prints a line to stdout and then fork()s a child
which runs the update command via system() before terminating with exit(0).

When alfred's stdout is is buffered, at the time of the fork() the buffer
still holds the just-printed "executing: ..." line along with any other
pending output, and the child receives a copy of that buffer. Calling
exit(0) in the child flushes its copy, and the parent flushes the same
content later, so every log line still buffered at fork() time is written
out twice.

Terminate the child with _exit(0) instead, which does not flush the stdio
buffers inherited from the parent, leaving the buffered output to be
written exactly once by the parent.

Fixes: 4ae4c749c0a4 ("alfred: Add "--update-command" parameter")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server.c b/server.c
index cb3fcfe..47613fd 100644
--- a/server.c
+++ b/server.c
@@ -364,7 +364,7 @@ static void execute_update_command(struct globals *globals)
 	script_pid = fork();
 	if (script_pid == 0) {
 		system(command);
-		exit(0);
+		_exit(0);
 	}
 
 	free(command);

-- 
2.47.3

[PATCH 2/2] alfred: Announce changed data only after it was stored

[Sven Eckelmann]

The event listeners were notified and the data type was marked as changed
before the buffer for the new data was allocated. When that allocation
fails then the dataset is left behind with a zero length and without
buffer. But "alfred -E" has already reported the update and the update
command will be started for a data type which was never stored (and which
lost its previous content).

Only notify the listeners and mark the type as changed after the new
data was copied into the dataset. And only free the old buffer when the new
was allocated.

Fixes: b106a7a36f98 ("alfred: notify event listener via unix socket")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 recv.c      | 41 +++++++++++++++++++++++++----------------
 unix_sock.c | 43 ++++++++++++++++++++++++++++---------------
 2 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/recv.c b/recv.c
index 76bfb30..929d086 100644
--- a/recv.c
+++ b/recv.c
@@ -32,8 +32,10 @@ static int finish_alfred_push_data(struct globals *globals,
 	struct dataset *dataset;
 	bool new_entry_created;
 	unsigned int data_len;
+	unsigned char *buf;
 	unsigned int len;
 	uint8_t *pos;
+	bool changed;
 
 	/* test already done in process_alfred_push_data */
 	len = ntohs(push->header.length);
@@ -62,10 +64,6 @@ static int finish_alfred_push_data(struct globals *globals,
 			dataset->data_source = SOURCE_SYNCED;
 
 			memcpy(&dataset->data, data, sizeof(*data));
-			if (hash_add(globals->data_hash, dataset)) {
-				free(dataset);
-				goto err;
-			}
 			new_entry_created = true;
 		}
 		/* don't overwrite our own data */
@@ -75,13 +73,14 @@ static int finish_alfred_push_data(struct globals *globals,
 		clock_gettime(CLOCK_MONOTONIC, &dataset->last_seen);
 
 		/* check that data was changed */
-		if (new_entry_created ||
-		    dataset->data.header.length != data_len ||
-		    memcmp(dataset->buf, data->data, data_len) != 0) {
-			changed_data_type(globals, data->header.type);
-			unix_sock_event_notify(globals, data->header.type,
-					       data->source);
-		}
+		changed = new_entry_created ||
+			  dataset->data.header.length != data_len ||
+			  !dataset->buf ||
+			  memcmp(dataset->buf, data->data, data_len) != 0;
+
+		buf = malloc(data_len);
+		if (!buf)
+			goto err;
 
 		/* free old buffer */
 		if (dataset->buf) {
@@ -89,16 +88,26 @@ static int finish_alfred_push_data(struct globals *globals,
 			dataset->data.header.length = 0;
 		}
 
-		dataset->buf = malloc(data_len);
-
-		/* that's not good */
-		if (!dataset->buf)
-			goto err;
+		if (new_entry_created) {
+			if (hash_add(globals->data_hash, dataset)) {
+				free(buf);
+				free(dataset);
+				goto err;
+			}
+		}
 
+		dataset->buf = buf;
 		dataset->data.header.length = data_len;
 		dataset->data.header.version = data->header.version;
 		memcpy(dataset->buf, data->data, data_len);
 
+		/* only announce the new data once it is actually stored */
+		if (changed) {
+			changed_data_type(globals, data->header.type);
+			unix_sock_event_notify(globals, data->header.type,
+					       data->source);
+		}
+
 		/* if the sender is also the the source of the dataset, we
 		 * got a first hand dataset. */
 		if (memcmp(&mac, data->source, ETH_ALEN) == 0)
diff --git a/unix_sock.c b/unix_sock.c
index e6b44da..28752ec 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -106,8 +106,10 @@ static int unix_sock_add_data(struct globals *globals,
 	struct interface *interface;
 	struct alfred_data *data;
 	struct dataset *dataset;
+	unsigned char *buf;
 	int data_len;
 	int ret = -1;
+	bool changed;
 	int len;
 
 	len = ntohs(push->header.length);
@@ -166,34 +168,45 @@ static int unix_sock_add_data(struct globals *globals,
 		dataset->buf = NULL;
 
 		memcpy(&dataset->data, data, sizeof(*data));
-		if (hash_add(globals->data_hash, dataset)) {
-			free(dataset);
-			goto err;
-		}
 		new_entry_created = true;
 	}
 	dataset->data_source = SOURCE_LOCAL;
 	clock_gettime(CLOCK_MONOTONIC, &dataset->last_seen);
 
 	/* check that data was changed */
-	if (new_entry_created ||
-	    dataset->data.header.length != data_len ||
-	    memcmp(dataset->buf, data->data, data_len) != 0)
-		unix_sock_event_notify(globals, data->header.type,
-				       data->source);
+	changed = new_entry_created ||
+		  dataset->data.header.length != data_len ||
+		  !dataset->buf ||
+		  memcmp(dataset->buf, data->data, data_len) != 0;
 
-	/* free old buffer */
-	free(dataset->buf);
-
-	dataset->buf = malloc(data_len);
-	/* that's not good */
-	if (!dataset->buf)
+	buf = malloc(data_len);
+	if (!buf)
 		goto err;
 
+	/* free old buffer */
+	if (dataset->buf) {
+		free(dataset->buf);
+		dataset->data.header.length = 0;
+	}
+
+	if (new_entry_created) {
+		if (hash_add(globals->data_hash, dataset)) {
+			free(buf);
+			free(dataset);
+			goto err;
+		}
+	}
+
+	dataset->buf = buf;
 	dataset->data.header.length = data_len;
 	dataset->data.header.version = data->header.version;
 	memcpy(dataset->buf, data->data, data_len);
 
+	/* only announce the new data once it is actually stored */
+	if (changed)
+		unix_sock_event_notify(globals, data->header.type,
+				       data->source);
+
 	ret = 0;
 err:
 	close(client_sock);

-- 
2.47.3