Re: [PATCH] alfred: notify event listener via unix socket
On Sunday, 1 May 2022 11:10:13 CEST Marek Lindner wrote:
> > > +static void unix_sock_event_listener_free(struct event_listener
> > > *listener)
> > > +{
> > > + list_del_init(>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
>
> What are those downsides you are referring to ?
Additional writes for something which is dropped anyway.
Kind regards,
Sven
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] alfred: notify event listener via unix socket
On Sunday, 1 May 2022 09:54:26 CEST Sven Eckelmann wrote:
> > 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?
Copy & paste error from server-status which does not require an argument
either. Might need to fix in a separate patch.
> 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?
Currently, no use-case comes to my mind.
> > +static void unix_sock_event_listener_free(struct event_listener
> > *listener)
> > +{
> > + list_del_init(>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
What are those downsides you are referring to ?
Thanks for the review!
Kind regards,
Marek Lindner
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] alfred: notify event listener via unix socket
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, >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(>list);
> + list_add_tail(>list, >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(>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, >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
signature.asc
Description: This is a digitally signed message part.
[PATCH] alfred: notify event listener via unix socket
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.
The actual data itself needs to be retrieved via the existing data
retrieval mechanisms.
Unlike the update-command this event monitor allows:
- multiple parallel listeners
- programmatic access to changes without requiring multiple processes
The alfred client allows to monitor events via the newly added '-E'
(event monitor) command line option. Serving as debugging tool and
example code at the same time.
Signed-off-by: Marek Lindner
---
alfred.h | 15 ++
client.c | 54
main.c | 10 +++-
man/alfred.8 | 3 ++
packet.h | 26 ++
recv.c | 4 +-
server.c | 5 ++
unix_sock.c | 142 +++
8 files changed, 257 insertions(+), 2 deletions(-)
diff --git a/alfred.h b/alfred.h
index 2d98a30..f442c48 100644
--- a/alfred.h
+++ b/alfred.h
@@ -94,6 +94,7 @@ enum clientmode {
CLIENT_CHANGE_INTERFACE,
CLIENT_CHANGE_BAT_IFACE,
CLIENT_SERVER_STATUS,
+ CLIENT_EVENT_MONITOR,
};
struct interface {
@@ -110,8 +111,15 @@ struct interface {
struct list_head list;
};
+struct event_listener {
+ int fd;
+
+ struct list_head list;
+};
+
struct globals {
struct list_head interfaces;
+ struct list_head event_listeners;
char *net_iface;
struct server *best_server; /* NULL if we are a server ourselves */
@@ -157,6 +165,7 @@ int alfred_client_modeswitch(struct globals *globals);
int alfred_client_change_interface(struct globals *globals);
int alfred_client_change_bat_iface(struct globals *globals);
int alfred_client_server_status(struct globals *globals);
+int alfred_client_event_monitor(struct globals *globals);
/* recv.c */
int recv_alfred_packet(struct globals *globals, struct interface *interface,
int recv_sock);
@@ -186,6 +195,12 @@ int unix_sock_open_client(struct globals *globals);
int unix_sock_close(struct globals *globals);
int unix_sock_req_data_finish(struct globals *globals,
struct transaction_head *head);
+int unix_sock_events_select_prepare(struct globals *globals, fd_set *fds,
+ fd_set *errfds, int maxsock);
+void unix_sock_events_select_handle(struct globals *globals,
+ fd_set *fds, fd_set *errfds);
+void unix_sock_events_close_all(struct globals *globals);
+void unix_sock_event_notify(struct globals *globals, uint8_t type);
/* vis.c */
int vis_update_data(struct globals *globals);
/* netsock.c */
diff --git a/client.c b/client.c
index 81cdd7c..9e88f47 100644
--- a/client.c
+++ b/client.c
@@ -452,3 +452,57 @@ err:
unix_sock_close(globals);
return 0;
}
+
+int alfred_client_event_monitor(struct globals *globals)
+{
+ struct alfred_event_register_v0 event_register;
+ struct alfred_event_notify_v0 event_notify;
+ int ret, len;
+
+ if (unix_sock_open_client(globals))
+ return -1;
+
+ len = sizeof(event_register);
+
+ event_register.header.type = ALFRED_EVENT_REGISTER;
+ event_register.header.version = ALFRED_VERSION;
+ event_register.header.length = 0;
+
+ ret = write(globals->unix_sock, _register, len);
+ if (ret != len) {
+ fprintf(stderr, "%s: only wrote %d of %d bytes: %s\n",
+ __func__, ret, len, strerror(errno));
+ goto err;
+ }
+
+ while (true) {
+ len = read(globals->unix_sock, _notify,
sizeof(event_notify));
+ if (len == 0) {
+ fprintf(stdout, "Server closed the connection\n");
+ goto err;
+ }
+
+ if (len < 0) {
+ perror("read from unix socket failed");
+ goto err;
+ }
+
+ if (len != sizeof(event_notify)) {
+ fprintf(stderr, "notify read bytes: %d (expected:
%zu)\n",
+ len, sizeof(event_notify));
+ goto err;
+ }
+
+ if (event_notify.header.version != ALFRED_VERSION)
+ continue;
+
+ if (event_notify.header.type != ALFRED_EVENT_NOTIFY)
+ continue;
+
+ fprintf(stdout, "Event: type = %d\n", event_notify.type);
+ }
+
+err:
+ unix_sock_close(globals);
+ return 0;
+}
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
