[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/N] qemu driver FreeBSD support: fix compilation



  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

Attachment: pgpWRBDnZUeId.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]