Re: [libvirt] [PATCH v0] qemu driver FreeBSD support

On Sun, Dec 09, 2012 at 09:17:01PM +0400, Roman Bogorodskiy wrote: > Hello, > > Attached an initial version of the patch providing FreeBSD support for > qemu driver. Initial discussion on the topic started here: > > https://www.redhat.com/archives/libvir-list/2012-November/msg00841.html > > Roman Bogorodskiy The diffstat is configure.ac | 18 src/Makefile.am | 18 src/cpu/cpu_x86.c | 4 src/network/bridge_driver.c | 3 src/network/bsd_bridge_driver.c | 4265 ++++++++++++++++++++++++++++++++++++++++ src/network/bsd_bridge_driver.h | 67 src/network/default.xml | 2 src/nodeinfo.c | 62 src/qemu/qemu_capabilities.c | 7 src/qemu/qemu_command.c | 5 src/qemu/qemu_conf.c | 2 src/qemu/qemu_driver.c | 10 src/qemu/qemu_process.c | 14 src/rpc/virnetsocket.c | 31 src/util/bsd_virnetdevbridge.c | 522 ++++ src/util/bsd_virnetdevbridge.h | 54 src/util/bsd_virnetdevtap.c | 335 +++ src/util/bsd_virnetdevtap.h | 62 src/util/processinfo.c | 22 src/util/virinitctl.c | 2 src/util/virnetdev.c | 225 ++ 21 files changed, 5712 insertions(+), 18 deletions(-) As Eric mentioned, I'd really like to see 1 patch per logical change. This doc I wrote up to help openstack developers contains useful background info on why it is important: http://wiki.openstack.org/GitCommitMessages#Structural_split_of_changes I'm also not a fan of seeing entire source files copy+paste duplicated for BSD, when it appears that >= 95% of their contents are the same as the original file. A custom BSD file is only justified if the new code shares little-to- nothing with the existing code. Otherwise just #ifdef it. Given that I'm not reviewing any part of the patch related to the new bsd_XXXX files. > diff --git a/configure.ac b/configure.ac > index a695e52..0467a2a 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -360,10 +360,11 @@ dnl are also linux specific. The "network" and storage_fs drivers are known > dnl to not work on MacOS X presently, so we also make a note if compiling > dnl for that > > -with_linux=no with_osx=no > +with_linux=no with_osx=no with_freebsd=no > case $host in > *-*-linux*) with_linux=yes ;; > *-*-darwin*) with_osx=yes ;; > + *-*-freebsd*) with_freebsd=yes ;; > esac > > if test $with_linux = no; then > @@ -379,6 +380,7 @@ if test $with_linux = no; then > fi > > AM_CONDITIONAL([WITH_LINUX], [test "$with_linux" = "yes"]) > +AM_CONDITIONAL([WITH_FREEBSD], [test "$with_freebsd" = "yes"]) > > dnl Allow to build without Xen, QEMU/KVM, test or remote driver > AC_ARG_WITH([xen], > @@ -533,6 +535,10 @@ AC_DEFINE_UNQUOTED([IP6TABLES_PATH], "$IP6TABLES_PATH", [path to ip6tables binar > AC_PATH_PROG([EBTABLES_PATH], [ebtables], /sbin/ebtables, [/usr/sbin:$PATH]) > AC_DEFINE_UNQUOTED([EBTABLES_PATH], "$EBTABLES_PATH", [path to ebtables binary]) > > +if test "$with_freebsd" = "yes"; then > + AC_PATH_PROG([IFCONFIG_PATH], [ifconfig], /sbin/ifconfig, [/usr/sbin:$PATH]) > + AC_DEFINE_UNQUOTED([IFCONFIG_PATH], "$IFCONFIG_PATH", [path to ifconfig binary]) > +fi > > dnl > dnl Checks for the OpenVZ driver > @@ -949,11 +955,13 @@ fi > dnl > dnl check for kernel headers required by src/bridge.c > dnl > -if test "$with_qemu" = "yes" || test "$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 with QEMU or LXC support])) > -fi > > +if test "$with_linux" = "yes"; then > + if test "$with_qemu" = "yes" || test "$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 with QEMU or LXC support])) > + fi > +fi These changes all look fine - it in patches with the associated .c file changes that deal with it. > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c > index ca8cd92..d6af160 100644 > --- a/src/cpu/cpu_x86.c > +++ b/src/cpu/cpu_x86.c > @@ -40,7 +40,11 @@ > > static const struct cpuX86cpuid cpuidNull = { 0, 0, 0, 0, 0 }; > > +#if defined(__FreeBSD__) > +static const char *archs[] = { "i386", "amd64" }; > +#else > static const char *archs[] = { "i686", "x86_64" }; > +#endif This change, another few elsewhere, and existing arch code, all make me think that it is about time we stopped using architecture strings everywhere in the code. We should have an enum for valid arches, and then APIs to convert to/from the OS specific values as needed. As it is today, there are too many places checks specific arch strings in the code and it is easy for us to miss one that is relevant when porting to BSD.

> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 00cffee..d959db4 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -41,6 +41,9 @@ > #include <stdio.h> > #include <sys/wait.h> > #include <sys/ioctl.h> > +#ifdef __FreeBSD__ > +#include <sys/socket.h> > +#endif > #include <net/if.h> I'd like to see the error message you get without this include before ACK'ing this. Use of GNULIB means we ought to never need to do conditional includes of standardized header files like socket.h