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

[Eric Blake]

On 08/28/2012 09:17 PM, Doug Goldstein wrote:
> <not part of the commit>
> Really just looking for feedback if this an acceptable update to the previous work. My plan is to add a 'udev' backend that will provide just some basic read-only host interface information. Basically I've been seeing postings to libvirt-users and virt-tools where people are on distros that don't support netcf and are noticing that virt-manager uses HAL or they use libvir APIs and can configure everything for the virtual machine respecting the host except for the network.
> </not part of the commit>

It's best to stick comments like this...

> 
> Based exclusively on work by Eric Blake in a patch posted with the same
> subject. However some modifications related to comments and my plans to
> add another backend.

Thanks for picking up work on my old attempt.

> 
> Added WITH_INTERFACE as the only automake variable deciding whether to
> build the driver and using WITH_NETCF to identify that we're wanting to
> use the netcf library as the backend.
> 
> * configure.ac: Added with_interface and enhanced with_netcf to respect
> with_interface.

This sounds backwards.  If I recall the discussion back on my old
attempt, the consensus moved towards wanting all library checks first,
and all driver checks last.  But I'll have to look at the actual
configure changes to see if they make sense.

> * src/interface/netcf_driver.c: Renamed..
> * src/interface/interface_backend_netcf.c: ..to this to match storage.
> * src/interface/netcf_driver.h: Renamed..
> * src/interface/interface_driver.h: ..to this.
> * daemon/Makefile.am: Respect WITH_INTERFACE and WITH_NETCF.
> ---

...here, after the '---' marker.  Then 'git am' will automatically
discard them, while they will still be available as a review hint to the
reader.

>  src/Makefile.am                         |   24 +-
>  src/interface/interface_backend_netcf.c |  663 +++++++++++++++++++++++++++++++
>  src/interface/interface_driver.h        |   29 ++
>  src/interface/netcf_driver.c            |  663 -------------------------------
>  src/interface/netcf_driver.h            |   29 --

'git config diff.renames true', then this will output a MUCH more
compact patch.

> +++ b/configure.ac
> @@ -1963,9 +1963,6 @@ 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])

This delays the netcf probe...

>  
>  
>  AC_ARG_WITH([secrets],
> @@ -2806,6 +2803,46 @@ if test "$with_nwfilter" = "yes" ; then
>  fi
>  AM_CONDITIONAL([WITH_NWFILTER], [test "$with_nwfilter" = "yes"])
>  
> +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])
> +
> +dnl Don't compile the interface driver without libvirtd
> +if test "$with_libvirtd" = "no" ; then
> +  with_interface=no
> +fi
> +
> +dnl The interface driver depends on the netcf library
> +if test "$with_interface:$with_netcf" = "check:yes" ; then
> +  with_interface=yes
> +fi

...but this tries to make decisions based on the probe...

> +
> +if test "$with_interface:$with_netcf" = "check:no" ; then
> +  with_interface=no
> +fi
> +
> +if test "$with_interface:$with_netcf" = "yes:no" ; then
> +  AC_MSG_ERROR([Requested the Interface driver without netcf support])
> +fi
> +
> +if test "$with_interface" = "yes" ; then
> +  AC_DEFINE_UNQUOTED([WITH_INTERFACE], [1],
> +    [whether the interface driver is enabled])
> +fi
> +AM_CONDITIONAL([WITH_INTERFACE], [test "$with_interface" = "yes"])
> +
> +dnl If the interface driver is off disable netcf
> +if test "$with_interface" = "no" ; then
> +  with_netcf=no
> +fi
> +
> +dnl We only use netcf for the interface driver so only enable it then
> +AM_CONDITIONAL([WITH_NETCF], [test "$with_netcf" = "yes"])
> +AC_SUBST([NETCF_CFLAGS])
> +AC_SUBST([NETCF_LIBS])

...that doesn't occur until here.  So this logic needs to be rewritten.
 I think the correct order is to check netcf first, (add in your new
backend check at this point), and _finally_ check with_interface, using:

if test $with_interface = yes; then
    require at least one backend (for now, netcf) is also yes, or fail
else if test $with_interface = check; then
    if at least one backend is yes, then set yes
fi

> @@ -3018,6 +3055,7 @@ AC_MSG_NOTICE([   Remote: $with_remote])
>  AC_MSG_NOTICE([  Network: $with_network])
>  AC_MSG_NOTICE([ Libvirtd: $with_libvirtd])
>  AC_MSG_NOTICE([    netcf: $with_netcf])
> +AC_MSG_NOTICE([Interface: $with_interface])
>  AC_MSG_NOTICE([  macvtap: $with_macvtap])
>  AC_MSG_NOTICE([ virtport: $with_virtualport])
>  AC_MSG_NOTICE([])

I think there's still more data we should be outputting in this section,
as well as some re-ordering.  with_netcf should be output in the
libraries section, not the drivers section.

> +++ b/src/util/util.c
> @@ -1251,39 +1251,52 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED,
>  }
>  #endif /* WIN32 */
>  
> +#define here fprintf(stderr, "%s:%d %s\n", __func__, __LINE__, path)

leftover debugging?

> +
>  static int virFileMakePathHelper(char *path, mode_t mode)
>  {
>      struct stat st;
>      char *p;
>  
>      if (stat(path, &st) >= 0) {
> -        if (S_ISDIR(st.st_mode))
> +        if (S_ISDIR(st.st_mode)) {
> +            here;
>              return 0;
> +        }
>  
>          errno = ENOTDIR;
> +        here;
>          return -1;
...

Kill this hunk.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120831/47088b23/attachment-0001.sig>