Doug Goldstein wrote:

On Tue, Dec 11, 2012 at 1:08 PM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:

...

Another round of the FreeBSD patches. Here's the first chunk to fix compilation.

Roman Bogorodskiy

Not to nitpick, but if possible in the future send the patches inline to the mailing list. You can accomplish this by doing the following:

$ git config --edit (optionally add --global for your whole user account to change)

[sendemail] smtpserver = smtp.gmail.com smtpserverport = 587 smtpencryption = tls smtpuser = bogorodskiy@gmail.com

You can also do the following with git config --edit without --global.

[sendemail] to = libvir-list@redhat.com

Now you can just run the following in your branch: $ git send-email master

And your patches will go to the ML.

Good, thanks.

Now I'll paste your patch inline with some notes.

commit 4205ba22dcef12ac41cf11cf8fe2a84f42612154 Author: Roman Bogorodskiy <bogorodskiy@gmail.com> Date: Tue Dec 11 22:31:39 2012 +0400

Qemu FreeBSD: fix compilation

* Autotools changes: - Don't assume Qemu is Linux-only - Check Linux headers only on Linux - Disable firewalld on FreeBSD * Initctl: Initctl seem to present only on Linux, so stub it on other platforms * Raw I/O: Linux-only as well * Headers cleanup

diff --git a/configure.ac b/configure.ac index a695e52..b29b65d 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 @@ -371,14 +372,15 @@ if test $with_linux = no; then then with_lxc=no fi - if test "x$with_qemu" != xyes - then - with_qemu=no - fi

This would appear to cause undesired results on Mac OS X.

I see, I'll add a macos check also.

with_dtrace=no fi

+if test $with_freebsd = yes; then + with_firewalld=no +fi +

This should be unnecessary. We should be checking whether we want to enable this or not and opting not to if the check fails.

Actually, it gets enabled if dbus is available: if test "x$with_firewalld" = "xcheck" ; then with_firewalld=$with_dbus fi Since FreeBSD has dbus, it also gets enabled.

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], @@ -949,9 +951,11 @@ 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])) +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

@@ -2880,14 +2884,24 @@ if test "x$with_libblkid" = "xyes"; then fi AM_CONDITIONAL([HAVE_LIBBLKID], [test "x$with_libblkid" = "xyes"])

+if test $with_freebsd = yes; then + default_qemu_user=root + default_qemu_group=wheel +else + default_qemu_user=root + default_qemu_group=root +fi + AC_ARG_WITH([qemu-user], - AC_HELP_STRING([--with-qemu-user], [username to run QEMU system instance as @<:@default=root@:>@]), + AC_HELP_STRING([--with-qemu-user], + [username to run QEMU system instance as @<:@default=platform dependent@:>@]), [QEMU_USER=${withval}], - [QEMU_USER=root]) + [QEMU_USER=${default_qemu_user}]) AC_ARG_WITH([qemu-group], - AC_HELP_STRING([--with-qemu-group], [groupname to run QEMU system instance as @<:@default=root@:>@]), + AC_HELP_STRING([--with-qemu-group], + [groupname to run QEMU system instance as @<:@default=platform dependent@:>@]), [QEMU_GROUP=${withval}], - [QEMU_GROUP=root]) + [QEMU_GROUP=${default_qemu_group}]) AC_DEFINE_UNQUOTED([QEMU_USER], ["$QEMU_USER"], [QEMU user account]) AC_DEFINE_UNQUOTED([QEMU_GROUP], ["$QEMU_GROUP"], [QEMU group account])

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..e95609c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -34,7 +34,6 @@ #include <sys/wait.h> #include <arpa/inet.h> #include <sys/utsname.h> -#include <mntent.h>

#include "virterror_internal.h" #include "qemu_conf.h"

This probably belongs in its own commit.

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..5d355eb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -27,7 +27,12 @@ #include <sys/stat.h> #include <sys/time.h> #include <sys/resource.h> +#if defined(__linux__) #include <linux/capability.h> +#elif defined(__FreeBSD__) +#include <sys/param.h> +#include <sys/cpuset.h> +#endif

#include "qemu_process.h" #include "qemu_domain.h" @@ -3707,7 +3712,12 @@ int qemuProcessStart(virConnectPtr conn, /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */ for (i = 0; i < vm->def->ndisks; i++) { if (vm->def->disks[i]->rawio == 1) +#ifdef CAP_SYS_RAWIO virCommandAllowCap(cmd, CAP_SYS_RAWIO); +#else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Raw I/O is not supported on this platform")); +#endif }

It probably would be better to not allow VMs to be defined with rawio enabled if this the case. You'll want to make these changes in src/conf/domain_conf.c

I guess it's a subject for the future changes. Anyway, this piece is still necessary because FreeBSD doesn't have CAP_SYS_RAWIO defined, so it won't compile without that.

virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c index cdd3dc0..ae2f525 100644 --- a/src/util/virinitctl.c +++ b/src/util/virinitctl.c @@ -35,6 +35,7 @@

#define VIR_FROM_THIS VIR_FROM_INITCTL

+#if defined(__linux__) || (defined(__FreeBSD_kernel__) && !(defined(__FreeBSD__))) /* These constants & struct definitions are taken from * systemd, under terms of LGPLv2+ * @@ -161,3 +162,11 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#else +int virInitctlSetRunLevel(virInitctlRunLevel level ATTRIBUTE_UNUSED, + const char *vroot ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); + return -1; +} +#endif

Are these changes really necessary? This is from SystemD and it talks about BSD in the source.

FreeBSD doesn't seem to have initctl stuff, the source comments probably mean something like Debian/kFreeBSD etc. Anyway, that won't compile because MAXHOSTNAMELEN is not 64 on FreeBSD, so the verify(sizeof(struct virInitctlRequest) == 384); check fails. Roman Bogorodskiy