[2/4] alfred: Allow operating without any interface specified

Message ID 20220102113136.470299-2-mareklindner@neomailbox.ch (mailing list archive)
State Superseded, archived
Delegated to: Simon Wunderlich
Headers
Series [1/4] alfred: remove meaningless printf() call |

Commit Message

Marek Lindner Jan. 2, 2022, 11:31 a.m. UTC
  The '-i' commandline parameter to specify interface names
no longer is mandatory. Specifying interface 'none' or
sending a 'none' interface string within the
ALFRED_CHANGE_INTERFACE unix socket command disables
all interfaces operations at runtime.

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 README.rst   |  6 +++++-
 alfred.h     |  1 -
 man/alfred.8 |  6 +++++-
 netsock.c    | 23 ++++-------------------
 server.c     | 12 ------------
 5 files changed, 14 insertions(+), 34 deletions(-)
  

Comments

Sven Eckelmann Jan. 2, 2022, 2:20 p.m. UTC | #1
On Sunday, 2 January 2022 12:31:34 CET Marek Lindner wrote:
[...]
> diff --git a/README.rst b/README.rst
[...]
>  	if (strcmp(globals->mesh_iface, "none") != 0 &&
>  	    batadv_interface_check(globals->mesh_iface) < 0 &&
>  	    !globals->force) {
> @@ -393,12 +387,6 @@ int alfred_server(struct globals *globals)
>  		return -1;
>  	}
>  
> -	num_socks = netsock_open_all(globals);
> -	if (num_socks <= 0 && !globals->force) {
> -		fprintf(stderr, "Failed to open interfaces\n");
> -		return -1;
> -	}
> -
>  	num_interfaces = netsocket_count_interfaces(globals);
>  	if (num_interfaces > 1 && globals->opmode == OPMODE_SECONDARY) {
>  		fprintf(stderr, "More than one interface specified in secondary mode\n");
> 

This now causes the "--force" option (+its storage in the globals data 
structure) to be completely useless. I would prefer to have this handling
still be there when !list_empty(&globals->interfaces).

And why is it necessary to not open the sockets on startup when interfaces are 
already given?

Kind regards,
	Sven
  
Marek Lindner Jan. 2, 2022, 7:01 p.m. UTC | #2
On Sunday, 2 January 2022 15:20:20 CET Sven Eckelmann wrote:
> This now causes the "--force" option (+its storage in the globals data
> structure) to be completely useless. 

Why would global->force be useless ? The alfred_server() function still uses 
the global->force state to determine if globals->mesh_iface is configured 
correctly.


> I would prefer to have this handling still be there when
> !list_empty(&globals->interfaces).

To be honest, I hadn't fully understood what use case global->force is trying 
to address. What do you have in mind ? Checking for list_empty() will require 
alfred to be always started with an interface configured while alfred could be 
used without any interface at all and operate as local data storage between 2 
processes on the same system or the interface could be configured at a later 
time (via unix socket).


> And why is it necessary to not open the sockets on startup when interfaces
> are already given?

The main while loop calls netsock_reopen() in each round which will open all 
necessary sockets (unless I am mistaken). My guess is that this was added when 
the ALFRED_CHANGE_INTERFACE call was added. Therefore, the netsock_open() call 
is somewhat redundant unless alfred is meant to always require an interface at 
startup time and alfred is meant to bail out whenever that configured interface 
isn't available at startup time.

Cheers,
Marek
  
Sven Eckelmann Jan. 3, 2022, 8:54 a.m. UTC | #3
On Sunday, 2 January 2022 20:01:47 CET Marek Lindner wrote:
> On Sunday, 2 January 2022 15:20:20 CET Sven Eckelmann wrote:
> > This now causes the "--force" option (+its storage in the globals data
> > structure) to be completely useless. 
> 
> Why would global->force be useless ? The alfred_server() function still uses 
> the global->force state to determine if globals->mesh_iface is configured 
> correctly.

Ok, you are right about mesh_iface. But the patch missed to adjust the manpage 
to clarify this.

> > I would prefer to have this handling still be there when
> > !list_empty(&globals->interfaces).
> 
> To be honest, I hadn't fully understood what use case global->force is trying 
> to address. What do you have in mind ? Checking for list_empty() will require 
> alfred to be always started with an interface configured while alfred could be 
> used without any interface at all and operate as local data storage between 2 
> processes on the same system or the interface could be configured at a later 
> time (via unix socket).

No, I wasn't talking about list_empty() but about !list_empty(). You removed 
the first block because you want to have have alfred started without any 
interface - fine with that.

But the default behavior of alfred in the past was to first check if the 
selected interfaces make sense and then return an error if there was a problem 
to open them. The force option basically ignored any error when there was an 
interface not ready yet. But the patch completely removed the chance to pre-
check the interfaces on startup.

> > And why is it necessary to not open the sockets on startup when interfaces
> > are already given?
> 
> The main while loop calls netsock_reopen() in each round which will open all 
> necessary sockets (unless I am mistaken). My guess is that this was added when 
> the ALFRED_CHANGE_INTERFACE call was added. Therefore, the netsock_open() call 
> is somewhat redundant unless alfred is meant to always require an interface at 
> startup time and alfred is meant to bail out whenever that configured interface 
> isn't available at startup time.

See above. The situation I had in mind was:

* force not enabled
* an interface given to the process
* interface cannot be used by alfred

Kind regards,
	Sven
  

Patch

diff --git a/README.rst b/README.rst
index 33200e4..7f44db6 100644
--- a/README.rst
+++ b/README.rst
@@ -82,7 +82,8 @@  documentation how to configure alfred in this case. In any event, you can
 still run alfred from the command line. The relevant options are (for a full
 list of options, run alfred -h):
 
-  -i, --interface             specify the interface to listen on
+  -i, --interface             specify the interface to listen on. use 'none'
+                              to disable interface operations
   -b                          specify the batman-adv interface configured on
                               the system (default: bat0). use 'none' to disable
                               the batman-adv based best server selection
@@ -90,6 +91,9 @@  list of options, run alfred -h):
                               accepts data from secondaries and syncs it with
                               other primaries
 
+The interface option '-i' is optional. If interface 'none' is specified, the
+alfred daemon will not communicate with other alfred instances on the
+network unless the interface list is modified at runtime via the unix socket.
 The -b option is optional, and only needed if you run alfred on a batman-adv
 interface not called bat0, or if you don't use batman-adv at all
 (use '-b none'). In this case, alfred will still work but will not be able to
diff --git a/alfred.h b/alfred.h
index d844261..57d7daf 100644
--- a/alfred.h
+++ b/alfred.h
@@ -182,7 +182,6 @@  int unix_sock_req_data_finish(struct globals *globals,
 /* vis.c */
 int vis_update_data(struct globals *globals);
 /* netsock.c */
-int netsock_open_all(struct globals *globals);
 size_t netsocket_count_interfaces(struct globals *globals);
 void netsock_close_all(struct globals *globals);
 int netsock_set_interfaces(struct globals *globals, char *interfaces);
diff --git a/man/alfred.8 b/man/alfred.8
index 18e008e..ff9b315 100644
--- a/man/alfred.8
+++ b/man/alfred.8
@@ -95,12 +95,16 @@  Change the alfred server to use the new \fBinterface\fP(s)
 .SH SERVER OPTIONS
 .TP
 \fB\-i\fP, \fB\-\-interface\fP \fIiface\fP
-Specify the interface (or comma separated list of interfaces) to listen on
+Specify the interface (or comma separated list of interfaces) to listen on.
+Use 'none' to disable interface operations.
 .TP
 \fB\-b\fP \fIbatmanif\fP
 Specify the batman-adv interface configured on the system (default: bat0).
 Use 'none' to disable the batman-adv based best server selection.
 
+The interface option \fB\-i\fP is optional. If interface 'none' is specified, the
+alfred daemon will not communicate with other alfred instances on the
+network unless the interface list is modified at runtime via the unix socket.
 The \fB\-b\fP option is optional, and only needed if you run alfred on a
 batman-adv interface not called bat0, or if you don't use batman-adv at all
 (use '\fB\-b\fP none'). In this case, alfred will still work but will not be
diff --git a/netsock.c b/netsock.c
index 84b0ec3..7bc49b4 100644
--- a/netsock.c
+++ b/netsock.c
@@ -116,6 +116,10 @@  int netsock_set_interfaces(struct globals *globals, char *interfaces)
 
 	netsock_close_all(globals);
 
+	/* interface 'none' disables all interface operations */
+	if (strcmp(interfaces, "none") == 0)
+		return 0;
+
 	input = interfaces;
 	while ((token = strtok_r(input, ",", &saveptr))) {
 		input = NULL;
@@ -452,25 +456,6 @@  err:
 	return -1;
 }
 
-int netsock_open_all(struct globals *globals)
-{
-	int num_socks = 0;
-	int ret;
-	struct interface *interface;
-
-	list_for_each_entry(interface, &globals->interfaces, list) {
-		if (globals->ipv4mode)
-			ret = netsock_open4(interface);
-		else
-			ret = netsock_open(interface);
-
-		if (ret >= 0)
-			num_socks++;
-	}
-
-	return num_socks;
-}
-
 size_t netsocket_count_interfaces(struct globals *globals)
 {
 	struct interface *interface;
diff --git a/server.c b/server.c
index 85bf453..39206bf 100644
--- a/server.c
+++ b/server.c
@@ -372,7 +372,6 @@  int alfred_server(struct globals *globals)
 	struct timespec last_check, now, tv;
 	fd_set fds, errfds;
 	size_t num_interfaces;
-	int num_socks;
 
 	if (create_hashes(globals))
 		return -1;
@@ -380,11 +379,6 @@  int alfred_server(struct globals *globals)
 	if (unix_sock_open_daemon(globals))
 		return -1;
 
-	if (list_empty(&globals->interfaces)) {
-		fprintf(stderr, "Can't start server: interface missing\n");
-		return -1;
-	}
-
 	if (strcmp(globals->mesh_iface, "none") != 0 &&
 	    batadv_interface_check(globals->mesh_iface) < 0 &&
 	    !globals->force) {
@@ -393,12 +387,6 @@  int alfred_server(struct globals *globals)
 		return -1;
 	}
 
-	num_socks = netsock_open_all(globals);
-	if (num_socks <= 0 && !globals->force) {
-		fprintf(stderr, "Failed to open interfaces\n");
-		return -1;
-	}
-
 	num_interfaces = netsocket_count_interfaces(globals);
 	if (num_interfaces > 1 && globals->opmode == OPMODE_SECONDARY) {
 		fprintf(stderr, "More than one interface specified in secondary mode\n");