On Wed, Aug 13, 2008 at 03:17:09PM +0100, Daniel P. Berrange wrote:
> Over time we've added lots of general purpose source code files, as wel
> as making more of the existing ones conditionally compiled. We've done
> this by adding lots of #ifdef WITH_XXXX macros across the various
> source files. This is becoming rather a minefield with soo many conditionals
> sprinkled throughout the code.
>
> With all the recent re-factoring we now have very good separation between
> generic code, and driver specific code - each typically being in separate
> files. Thus it is now practical to remove the vast majority of the macros
> and just do the conditional compilation via the Makefile.am.
>
> The patch is not entirely clear - the resulting Makefile.am is much easier
> to review. It is structured in two section, first we declare variables
> for groups of source code files - eg generic helper files, generic domain
> XML files, generic network XML files, and then per-driver files.
In general this sounds good,
I don't like automake conditionals too much, but in that case yes, that's
logical.
[...]
> In removing the unneeded WITH_XXX macros from header/sources I noticed that
> a bunch of our header files have
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> With is irrelevant since we're not using C++ anywhere - indeed some of the
> files even had the corresponding '}' missing, so clearly this is never
> actually used during compilation. Removing this fixes bogus indentation
> in some of the header files
Well it should be kept in the public headers, but right internally
it's just remains of cut and paste.
> I've tested this all by running configure --without-XXX for each hypervisor
> driver in turn, and it seemed to work in all cases. The only thing I didn't
> test is MinGW. I think I really need to add a MinGW based build to the
> nightly build system, so we can sanity check that nightly
> --- a/configure.in Tue Aug 12 22:21:25 2008 +0100
> +++ b/configure.in Tue Aug 12 22:21:47 2008 +0100
> @@ -241,27 +241,35 @@
> LIBVIRT_FEATURES=
> WITH_XEN=0
>
> -if test "$with_openvz" = "yes" ; then
> +if test "$with_openvz" = "yes"; then
hum, unrelated :-)
> -if test "$with_qemu" = "yes" ; then
> +if test "$with_qemu" = "yes" -o "$with_lxc" = "yes" ; then
> AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],,
> AC_MSG_ERROR([You must install kernel-headers in order to compile libvirt]))
> fi
oh, good point !
[...]
> +if WITH_REMOTE
> +libvirt_la_SOURCES += $(REMOTE_DRIVER_SOURCES)
> +endif
I'm wondering about += portability, but I think this is widely used
and really clarifies the resulting Makefile.am
the other solution
libvirt_la_SOURCES =
....
if WITH_REMOTE
$(REMOTE_DRIVER_SOURCES)
endif
if WITH_XEN
....
might be a bit more portable but messes the structure,
No other comment, everything else is small bugs, the cleanup of headers
and reindent,
Fine by me, +1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list