On Saturday, 30 April 2022 12:56:47 CEST Marek Lindner wrote:
> The alfred server instance accepts event notification registration
> via the unix socket. These notification sockets only inform
> registered parties of the availibility of an alfred datatype change.

availability


> +int alfred_client_event_monitor(struct globals *globals)
> +{
[...]
> +		fprintf(stdout, "Event: type = %d\n", event_notify.type);

event_notify.type is unsigned and not signed.


> diff --git a/main.c b/main.c
> index 68d6efd..98bf64d 100644
> --- a/main.c
> +++ b/main.c
> @@ -39,6 +39,7 @@ static void alfred_usage(void)
>  	printf("  -I, --change-interface [interface]  change to the specified interface(s)\n");
>  	printf("  -B, --change-bat-iface [interface]  change to the specified batman-adv interface\n");
>  	printf("  -S, --server-status                 request server status info such as mode & interfaces\n");
> +	printf("  -E, --event-monitor                 monitor alfred data record update eventss\n");

events

>  	printf("\n");
>  	printf("server mode options:\n");
>  	printf("  -i, --interface                     specify the interface (or comma separated list of interfaces) to listen on\n");
> @@ -164,6 +165,7 @@ static struct globals *alfred_init(int argc, char *argv[])
>  		{"change-interface",	required_argument,	NULL,	'I'},
>  		{"change-bat-iface",	required_argument,	NULL,	'B'},
>  		{"server-status",	required_argument,	NULL,	'S'},
> +		{"event-monitor",	required_argument,	NULL,	'E'},


Why does it require an argument but the usage doesn't describe the argument? 
See also getopt_long which also doesn't expect an argument


> @@ -138,10 +140,17 @@ static int unix_sock_add_data(struct globals *globals,
>  			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);
> +

I am wondering now if it could be interesting for the listener whether the 
data is from us or some remote (for example via the source mac). Does anyone 
else have an opinion about that?




> +static int unix_sock_register_listener(struct globals *globals, int client_sock)
> +{
> +	struct event_listener *listener;
> +	int ret;
> +
> +	ret = fcntl(client_sock, F_GETFL, 0);
> +	if (ret < 0) {
> +		perror("failed to get file status flags");
> +		goto err;
> +	}
> +
> +	ret = fcntl(client_sock, F_SETFL, ret | O_NONBLOCK);
> +	if (ret < 0) {
> +		perror("failed to set file status flags");
> +		goto err;
> +	}
> +
> +	listener = malloc(sizeof(*listener));
> +	if (!listener)
> +		goto err;
> +
> +	listener->fd = client_sock;
> +	INIT_LIST_HEAD(&listener->list);
> +	list_add_tail(&listener->list, &globals->event_listeners);

INIT_LIST_HEAD (of the prev/next pointer) is not necessary when you just 
overwrite the next/prev pointer in the next line via list_add_tail

> +
> +static void unix_sock_event_listener_free(struct event_listener *listener)
> +{
> +	list_del_init(&listener->list);
> +	close(listener->fd);
> +	free(listener);

list_del_init has no benefit (only downsides) when you free the memory anyway 
at the end of the function


> +int unix_sock_events_select_prepare(struct globals *globals, fd_set *fds,
> +				    fd_set *errfds, int maxsock)
> +{
> +	struct event_listener *listener;
> +
> +	list_for_each_entry(listener, &globals->event_listeners, list) {
> +		if (listener->fd < 0)
> +			continue;
> +
> +		FD_SET(listener->fd, fds);
> +		FD_SET(listener->fd, errfds);
> +
> +		if (maxsock < listener->fd)
> +			maxsock = listener->fd;
> +	}
> +
> +	return maxsock;
> +}

I should really rewrite the socket mainloop using epoll.... but this is just a 
side note and not a problem in your patch.

Kind regards,
	Sven