[libvirt] [PATCH v0] qemu driver FreeBSD support
Jiri Denemark
jdenemar at redhat.com
Mon Dec 10 20:11:12 UTC 2012
On Mon, Dec 10, 2012 at 18:23:05 +0000, Daniel P. Berrange wrote:
> 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.
Not to mention that this particular change looks quite suspicious. I think we
only need to change arch names when we need to map between archs we get or
need to use when doing system calls. Libvirt itself should use the same arch
name for a single architecture both internally and externally in XMLs and
APIs. It does not make sense to me to call the same architecture differently
depending on the host OS. In particular, domain XML for qemu driver on Linux
should just work without modifications on FreeBSD (iff no Linux-specific
features are used, of course). And capabilities XML should also report
consistent architecture name independently on how host OS calls it.
...
> > 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
Also if sys/socket.h is required on FreeBSD, I don't see a reason to include
it only there. Making the include unconditional on all architectures seems
just fine to me as this is a standard header.
Jirka
More information about the libvir-list
mailing list