[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] build: define WITH_INTERFACE for the driver



On 2012年06月29日 07:57, Eric Blake wrote:
Our code was mistakenly relying on an undefined macro, WITH_INTERFACE,
for determining whether to load the interface driver which wraps the
netcf library.  Clean this situation up by having only one automake
conditional for the driver, and having both WITH_NETCF (library
detected) and WITH_INTERFACE (driver enabled) in C code, in case a
future patch ever adds a network management via means other than
the netcf library.

Foresighted. :-)


While at it, output more information at the conclusion of configure
about the various drivers we enabled.

* configure.ac: Enhance with_netcf, and add with_interface.
Improve output to list final decisions.  Replace WITH_NETCF with
WITH_INTERFACE.
* src/interface/netcf_driver.c: Rename...
* src/interface/interface_driver.c: ...to this.
* src/interface/interface_driver.h: Likewise.
* daemon/Makefile.am (libvirtd_LDADD): Reflect better naming.
* src/Makefile.am (libvirt_driver_interface_la_*): Likewise.
(INTERFACE_DRIVER_SOURCES): Reflect file moves.
* daemon/libvirtd.c (daemonInitialize): Likewise.
* tools/virsh.c (vshShowVersion): Show both driver and library
decisions.
* libvirt.spec.in (with_interface): Tweak to deal with new usage
as a real switch.
---

I think this addresses the point that Osier raised here:
https://www.redhat.com/archives/libvir-list/2012-June/msg01266.html

but it is complex enough that I'd appreciate a careful review.

  configure.ac                                       |   44 ++++++++++++++++----
  daemon/Makefile.am                                 |    2 +-
  daemon/libvirtd.c                                  |    6 +--
  libvirt.spec.in                                    |   10 +++--
  src/Makefile.am                                    |    4 +-
  .../{netcf_driver.c =>  interface_driver.c}         |    4 +-
  .../{netcf_driver.h =>  interface_driver.h}         |    0
  tools/virsh.c                                      |   11 +++--
  8 files changed, 59 insertions(+), 22 deletions(-)
  rename src/interface/{netcf_driver.c =>  interface_driver.c} (99%)
  rename src/interface/{netcf_driver.h =>  interface_driver.h} (100%)

diff --git a/configure.ac b/configure.ac
index 6436885..a29b3b2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1755,6 +1755,7 @@ if test "$with_network" = "yes" ; then
  fi
  AM_CONDITIONAL([WITH_NETWORK], [test "$with_network" = "yes"])

+dnl check whether helper code is needed for above selections
  with_bridge=no
  if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then
      with_bridge=yes
@@ -1762,16 +1763,31 @@ if test "$with_qemu:$with_lxc:$with_network" != "no:no:no"; then
  fi
  AM_CONDITIONAL([WITH_BRIDGE], [test "$with_bridge" = "yes"])

-dnl netcf library
+dnl check if the interface driver should be compiled
+
+AC_ARG_WITH([interface],
+  AC_HELP_STRING([--with-interface],
+    [with host interface driver @<:@default=check@:>@]),[],
+    [with_interface=check])

Do we have to expose "with-interface"? It will give the user
a logic question, pick "with-interface", or 'with-netcf', or
both, even more when we have other implementations of interface
driver in future. however, the logic is simple, and we do it
inside actually: as long as one implementation of the interface
driver is picked to compile, we have the WITH_INTERFACE. so IMHO
no need to give the user the simple logic question. :-)

+
+dnl there's no use compiling the interface driver without the libvirt daemon
+if test "$with_libvirtd" = "no"; then
+  with_interface=no
+fi

If we don't expose 'with-interface', we don't need this......

+
+dnl The interface driver depends on the netcf library
  AC_ARG_WITH([netcf],
    AC_HELP_STRING([--with-netcf], [libnetcf support to configure physical host network interfaces @<:@default=check@:>@]),
  [], [with_netcf=check])

  NETCF_CFLAGS=
  NETCF_LIBS=
-if test "$with_libvirtd" = "no" ; then
+if test "$with_libvirtd" = "no" || test "$with_interface" = "no"; then
    with_netcf=no
  fi
+if test "$with_interface:$with_netcf" = "yes:check"; then
+  with_netcf=yes
+fi
  if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then
    PKG_CHECK_MODULES(NETCF, netcf>= $NETCF_REQUIRED,
      [with_netcf=yes], [
@@ -1792,11 +1808,21 @@ if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then
      fi
    fi
  fi
-AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"])
  AC_SUBST([NETCF_CFLAGS])
  AC_SUBST([NETCF_LIBS])

+dnl Final decision on the interface driver
+if test "$with_interface" = "check"; then
+  with_interface=$with_netcf
+fi
+
+if test "$with_interface" = "yes" ; then
+  AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1],
+    [whether interface driver is enabled])
+fi
+AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])


And above changes, what we need is just:

if test "$with_netfs" = "yes" || "with_something_else" = "yes"; then
    AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1],
      [whether interface driver is enabled])
fi
AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])

+dnl Check whether the Secrets driver is needed
  AC_ARG_WITH([secrets],
    AC_HELP_STRING([--with-secrets], [with local secrets management driver @<:@default=yes@:>@]),[],[with_secrets=yes])

@@ -2807,11 +2833,12 @@ AC_MSG_NOTICE([     ESX: $with_esx])
  AC_MSG_NOTICE([ Hyper-V: $with_hyperv])
  AC_MSG_NOTICE([    Test: $with_test])
  AC_MSG_NOTICE([  Remote: $with_remote])
-AC_MSG_NOTICE([ Network: $with_network])
  AC_MSG_NOTICE([Libvirtd: $with_libvirtd])
-AC_MSG_NOTICE([   netcf: $with_netcf])

And no AC_MSG_NOTICE for $with_interface here, with keeping
$with_netcf.

-AC_MSG_NOTICE([ macvtap: $with_macvtap])
-AC_MSG_NOTICE([virtport: $with_virtualport])
+AC_MSG_NOTICE([ Network: $with_network])
+AC_MSG_NOTICE([   Iface: $with_interface])
+AC_MSG_NOTICE([ Secrets: $with_secrets])
+AC_MSG_NOTICE([ NodeDev: $with_nodedev])
+AC_MSG_NOTICE([NWfilter: $with_nwfilter])
  AC_MSG_NOTICE([])
  AC_MSG_NOTICE([Storage Drivers])
  AC_MSG_NOTICE([])
@@ -2977,6 +3004,9 @@ AC_MSG_NOTICE([     Readline: $lv_use_readline])
  AC_MSG_NOTICE([       Python: $with_python])
  AC_MSG_NOTICE([       DTrace: $with_dtrace])
  AC_MSG_NOTICE([        numad: $with_numad])
+AC_MSG_NOTICE([       bridge: $with_bridge])
+AC_MSG_NOTICE([      macvtap: $with_macvtap])
+AC_MSG_NOTICE([     virtport: $with_virtualport])
  AC_MSG_NOTICE([  XML Catalog: $XML_CATALOG_FILE])
  AC_MSG_NOTICE([  Init script: $with_init_script])
  AC_MSG_NOTICE([Console locks: $with_console_lock_files])
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 71e91cd..f0e422e 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -148,7 +148,7 @@ if WITH_NETWORK
      libvirtd_LDADD += ../src/libvirt_driver_network.la
  endif

-if WITH_NETCF
+if WITH_INTERFACE
      libvirtd_LDADD += ../src/libvirt_driver_interface.la
  endif

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 9c06344..a4f98cf 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -74,8 +74,8 @@
  # ifdef WITH_NETWORK
  #  include "network/bridge_driver.h"
  # endif
-# ifdef WITH_NETCF
-#  include "interface/netcf_driver.h"
+# ifdef WITH_INTERFACE
+#  include "interface/interface_driver.h"
  # endif
  # ifdef WITH_STORAGE
  #  include "storage/storage_driver.h"
@@ -400,7 +400,7 @@ static void daemonInitialize(void)
  # ifdef WITH_NETWORK
      networkRegister();
  # endif
-# ifdef WITH_NETCF
+# ifdef WITH_INTERFACE
      interfaceRegister();
  # endif
  # ifdef WITH_STORAGE
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 896ef51..26ea2d9 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -70,6 +70,7 @@

  # Then the secondary host drivers, which run inside libvirtd
  %define with_network       0%{!?_without_network:%{server_drivers}}
+%define with_interface     0%{!?_without_interface:%{server_drivers}}
  %define with_storage_fs    0%{!?_without_storage_fs:%{server_drivers}}
  %define with_storage_lvm   0%{!?_without_storage_lvm:%{server_drivers}}
  %define with_storage_iscsi 0%{!?_without_storage_iscsi:%{server_drivers}}
@@ -214,6 +215,7 @@
  # The logic is the same as in configure.ac
  %if ! %{with_libvirtd}
  %define with_network 0
+%define with_interface 0
  %define with_qemu 0
  %define with_lxc 0
  %define with_uml 0
@@ -267,9 +269,7 @@
  %define with_nodedev 0
  %endif

-%if %{with_netcf}
-%define with_interface 1
-%else
+%if !%{with_netcf}
  %define with_interface 0
  %endif

@@ -1056,6 +1056,9 @@ of recent versions of Linux (and other OSes).
  %define _without_network --without-network
  %endif

+%if ! %{with_interface}
+%define _without_interface --without-interface
+%endif
  %if ! %{with_storage_fs}
  %define _without_storage_fs --without-storage-fs
  %endif
@@ -1171,6 +1174,7 @@ autoreconf -if
             %{?_without_hyperv} \
             %{?_without_vmware} \
             %{?_without_network} \
+           %{?_without_interface} \
             %{?_with_rhel5_api} \
             %{?_without_storage_fs} \
             %{?_without_storage_lvm} \

So, no these changes either.

diff --git a/src/Makefile.am b/src/Makefile.am
index 2309984..3cfaf01 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -483,7 +483,7 @@ NETWORK_DRIVER_SOURCES =					\
  		network/bridge_driver.h network/bridge_driver.c

  INTERFACE_DRIVER_SOURCES =					\
-		interface/netcf_driver.h interface/netcf_driver.c
+		interface/interface_driver.h interface/interface_driver.c

  SECRET_DRIVER_SOURCES =						\
  		secret/secret_driver.h secret/secret_driver.c
@@ -924,7 +924,7 @@ EXTRA_DIST += network/default.xml



-if WITH_NETCF
+if WITH_INTERFACE
  if WITH_DRIVER_MODULES
  mod_LTLIBRARIES += libvirt_driver_interface.la
  else
diff --git a/src/interface/netcf_driver.c b/src/interface/interface_driver.c
similarity index 99%
rename from src/interface/netcf_driver.c
rename to src/interface/interface_driver.c

We might want to rename it again if there is new implementations
in future. But it needs changes anyway, so it's fine.

index 45e6442..4959c72 100644
--- a/src/interface/netcf_driver.c
+++ b/src/interface/interface_driver.c
@@ -2,7 +2,7 @@
   * interface_driver.c: backend driver methods to handle physical
   *                     interface configuration using the netcf library.
   *
- * Copyright (C) 2006-2011 Red Hat, Inc.
+ * Copyright (C) 2006-2012 Red Hat, Inc.
   *
   * This library is free software; you can redistribute it and/or
   * modify it under the terms of the GNU Lesser General Public
@@ -27,7 +27,7 @@

  #include "virterror_internal.h"
  #include "datatypes.h"
-#include "netcf_driver.h"
+#include "interface_driver.h"
  #include "interface_conf.h"
  #include "memory.h"

diff --git a/src/interface/netcf_driver.h b/src/interface/interface_driver.h
similarity index 100%
rename from src/interface/netcf_driver.h
rename to src/interface/interface_driver.h
diff --git a/tools/virsh.c b/tools/virsh.c
index 53d1825..c1e7010 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -20832,15 +20832,18 @@ vshShowVersion(vshControl *ctl ATTRIBUTE_UNUSED)
  #ifdef WITH_NETWORK
      vshPrint(ctl, " Network");
  #endif
-#ifdef WITH_BRIDGE
-    vshPrint(ctl, " Bridging");
-#endif
-#ifdef WITH_NETCF
+#ifdef WITH_INTERFACE
      vshPrint(ctl, " Interface");

And no this.

  #endif
  #ifdef WITH_NWFILTER
      vshPrint(ctl, " Nwfilter");
  #endif
+#ifdef WITH_BRIDGE
+    vshPrint(ctl, " Bridging");
+#endif
+#ifdef WITH_NETCF
+    vshPrint(ctl, " Netcf");
+#endif
  #ifdef WITH_VIRTUALPORT
      vshPrint(ctl, " VirtualPort");
  #endif

Regards,
Osier


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]