Re: [libvirt] [PATCH 10/41] remote: conditionalize IP socket usage in libvirtd daemon

2019-07-26 Thread Andrea Bolognani 
On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
[...]
> @@ -988,7 +1004,13 @@ int main(int argc, char **argv) {
>  int c;
>  char *tmp;
>  
> -c = getopt_long(argc, argv, "ldf:p:t:vVh", opts, );
> +c = getopt_long(argc, argv,
> +#ifdef ENABLE_IP
> +"ldf:p:t:vVh",
> +#else /* ! ENABLE_IP */
> +"df:p:t:vVh",
> +#endif /* ! ENABLE_IP */
> +opts, );

This looks pretty awful... Can you please do something like

  #ifdef ENABLE_IP
const char *optstr = "ldf:p:t:vVh";
  #else /* ! ENABLE_IP */
const char *optstr = "df:p:t:vVh";
  #endif /* ! ENABLE_IP */

  c = getopt_long(argc, argv, optstr, opts, );

instead?

[...]
> @@ -1003,9 +1025,11 @@ int main(int argc, char **argv) {
>  case 'd':
>  godaemon = 1;
>  break;

One extra empty line here for clarity, please...

> +#ifdef ENABLE_IP
>  case 'l':
>  ipsock = 1;
>  break;
> +#endif /* ! ENABLE_IP */

[...]
> +++ b/src/remote/remote_daemon_config.h
> @@ -41,21 +43,26 @@ struct daemonConfig {
>  
>  int auth_unix_rw;
>  int auth_unix_ro;

... and one here as well.


With the above addressed,

  Reviewed-by: Andrea Bolognani 


[...]
> +char **sasl_allowed_username_list;

I like this approach you've taken of completely eliminating structure
members when the corresponding feature is not compiled in, and in
fact I think we should use it more extensively: for example, we
should guard sasl_allowed_username_list with IF_SASL. Out of scope
for the patch series at hand, of course! :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 10/41] remote: conditionalize IP socket usage in libvirtd daemon

2019-07-23 Thread Daniel P . Berrangé 
Prepare for reusing libvirtd source to create other daemons by making
the use of IP sockets conditionally defined by the make rules.

The main libvirtd daemon will retain IP listen ability, but all the
driver specific daemons will be local UNIX sockets only. Apps needing
IP connectivity will connect via the libvirtd daemon which will proxy
to the driver specfic daemon.

Signed-off-by: Daniel P. Berrangé 
---
 src/remote/Makefile.inc.am|  1 +
 src/remote/remote_daemon.c| 39 ++-
 src/remote/remote_daemon_config.c | 36 
 src/remote/remote_daemon_config.h |  9 ++-
 4 files changed, 68 insertions(+), 17 deletions(-)

diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
index b72186109a..2277bf49d2 100644
--- a/src/remote/Makefile.inc.am
+++ b/src/remote/Makefile.inc.am
@@ -148,6 +148,7 @@ libvirtd_CFLAGS = \
-I$(srcdir)/rpc \
-DSOCK_PREFIX="\"libvirt\"" \
-DDAEMON_NAME="\"libvirtd\"" \
+   -DENABLE_IP \
$(NULL)
 
 libvirtd_LDFLAGS = \
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index 2abeb08e16..45b52af987 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -371,11 +371,13 @@ static int ATTRIBUTE_NONNULL(3)
 daemonSetupNetworking(virNetServerPtr srv,
   virNetServerPtr srvAdm,
   struct daemonConfig *config,
+#ifdef ENABLE_IP
+  bool ipsock,
+  bool privileged,
+#endif /* ! ENABLE_IP */
   const char *sock_path,
   const char *sock_path_ro,
-  const char *sock_path_adm,
-  bool ipsock,
-  bool privileged)
+  const char *sock_path_adm)
 {
 gid_t unix_sock_gid = 0;
 int unix_sock_ro_mask = 0;
@@ -387,15 +389,19 @@ daemonSetupNetworking(virNetServerPtr srv,
 { .name = DAEMON_NAME ".socket", .family = AF_UNIX, .path = sock_path 
},
 { .name = DAEMON_NAME "-ro.socket", .family = AF_UNIX, .path = 
sock_path_ro },
 { .name = DAEMON_NAME "-admin.socket", .family = AF_UNIX, .path = 
sock_path_adm },
+#ifdef ENABLE_IP
 { .name = DAEMON_NAME "-tcp.socket", .family = AF_INET },
 { .name = DAEMON_NAME "-tls.socket", .family = AF_INET },
+#endif /* ! ENABLE_IP */
 };
 
+#ifdef ENABLE_IP
 if ((actmap[3].port = virSocketAddrResolveService(config->tcp_port)) < 0)
 return -1;
 
 if ((actmap[4].port = virSocketAddrResolveService(config->tls_port)) < 0)
 return -1;
+#endif /* ! ENABLE_IP */
 
 if (virSystemdGetActivation(actmap, ARRAY_CARDINALITY(actmap), ) < 0)
 return -1;
@@ -460,6 +466,7 @@ daemonSetupNetworking(virNetServerPtr srv,
config->admin_max_client_requests) < 0)
 goto cleanup;
 
+#ifdef ENABLE_IP
 if (((ipsock && config->listen_tcp) || act) &&
 virNetServerAddServiceTCP(srv,
   act,
@@ -534,6 +541,7 @@ daemonSetupNetworking(virNetServerPtr srv,
 }
 virObjectUnref(ctxt);
 }
+#endif /* ! ENABLE_IP */
 
 if (act &&
 virSystemdActivationComplete(act) < 0)
@@ -880,7 +888,9 @@ daemonUsage(const char *argv0, bool privileged)
 { "-h | --help", N_("Display program help") },
 { "-v | --verbose", N_("Verbose messages") },
 { "-d | --daemon", N_("Run as a daemon & write PID file") },
+#ifdef ENABLE_IP
 { "-l | --listen", N_("Listen for TCP/IP connections") },
+#endif /* ENABLE_IP */
 { "-t | --timeout ", N_("Exit after timeout period") },
 { "-f | --config ", N_("Configuration file") },
 { "-V | --version", N_("Display version information") },
@@ -917,6 +927,7 @@ daemonUsage(const char *argv0, bool privileged)
 LOCALSTATEDIR "/run/libvirt/" SOCK_PREFIX "-sock-ro");
 fprintf(stderr, "\n");
 
+#ifdef ENABLE_IP
 fprintf(stderr, "%s:\n", _("TLS"));
 fprintf(stderr, "  %s: %s\n",
 _("CA certificate"),
@@ -928,6 +939,7 @@ daemonUsage(const char *argv0, bool privileged)
 _("Server private key"),
 privileged ? LIBVIRT_SERVERKEY : 
"$HOME/.pki/libvirt/serverkey.pem");
 fprintf(stderr, "\n");
+#endif /* ENABLE_IP */
 
 fprintf(stderr, "%s:\n",
 _("PID file (unless overridden by -p)"));
@@ -954,7 +966,9 @@ int main(int argc, char **argv) {
 int timeout = -1;/* -t: Shutdown timeout */
 int verbose = 0;
 int godaemon = 0;
+#ifdef ENABLE_IP
 int ipsock = 0;
+#endif /* ! ENABLE_IP */
 struct daemonConfig *config;
 bool privileged = geteuid() == 0 ? true : false;
 bool implicit_conf = false;
@@ -964,7 +978,9 @@ int main(int argc, char **argv) {
 struct option opts[] = {
 { "verbose", no_argument, , 'v'},
 { "daemon", no_argument, , 'd'},
+#ifdef ENABLE_IP
 {