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

26 Jul 2019


      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, &optidx);
+        c = getopt_long(argc, argv,
+#ifdef ENABLE_IP
+                        "ldf:p:t:vVh",
+#else /* ! ENABLE_IP */
+                        "df:p:t:vVh",
+#endif /* ! ENABLE_IP */
+                        opts, &optidx);
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, &optidx);

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 <abologna@redhat.com>


[...]
...
+    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

    