[libvirt] [PATCH v0] qemu driver FreeBSD support

[libvirt] [PATCH v3 0/8] Allow...

[libvirt] [PATCH 0/8] parallels:...

Roman Bogorodskiy

Sunday, 9 December 2012 Sun, 9 Dec '12

11:17 a.m.

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

Attachments:

attachment.sig (application/pgp-signature — 488 bytes)
libvirt_qemu_freebsd_v0.diff (text/x-diff — 202.0 KB)

Show replies by date

Eric Blake

Monday, 10 December Mon, 10 Dec

11:50 a.m.

On 12/09/2012 10:17 AM, Roman Bogorodskiy wrote:

...

As I mentioned on IRC, it is extremely hard to review a monolithic patch that is 200k large [also, the list moderation flags any message larger than 150k, which adds moderation delay into the mix]. It would be much better to break this into multiple commits, each which touches a smaller task and can be reviewed in isolation (the intermediate results should pass 'make check' on all platforms, but you don't have to have working BSD support until the end of the series). I haven't looked at everything, but here's a few initial comments:

...

diff --git a/README-freebsd b/README-freebsd new file mode 100644 index 0000000..2c4079b

Why do you need a new file? What is in here that cannot be applied to the generic README?

...

@@ -0,0 +1,65 @@ +Configuring: + + ./configure --with-qemu --disable-werror --with-qemu-group=wheel --without-polkit --without-firewalld

How many of these configure options are the result of bad defaults in configure.ac? It would be better to patch configure.ac so that a default './configure' does the right thing, rather than requiring the user to pass lots of extra flags.

...

+To allow a VM have internet connection: + + +ipfw nat 1000 config if re0 + +ipfw add 1000 nat 1000 log ip from 192.168.122.0/24 to any out +ipfw add 1001 nat 1000 log ip from any to any in +

Why can't libvirt do this directly? How much of this work can be added to netcf, so that libvirt's wrappers around netcf just work?

...

+++ b/src/Makefile.am @@ -51,6 +51,14 @@ augeastest_DATA = # These files are not related to driver APIs. Simply generic # helper APIs for various purposes +if WITH_FREEBSD +NETDEV = util/bsd_virnetdevtap.h util/bsd_virnetdevtap.c \ + util/bsd_virnetdevbridge.h util/bsd_virnetdevbridge.c +else +NETDEV = util/virnetdevtap.h util/virnetdevtap.c \ + util/virnetdevbridge.h util/virnetdevbridge.c +endif

Yuck. New files should be in the vir* namespace, not the bsd_vir* namespace. But why do we even need new files? We should REALLY be making the single file virndetdevtap.h generic enough in its interface so that all other files just include the one header and it just works or has no-op stubs. It is okay to have more than one .c file but even then, you should mirror how we did src/util/threads-{pthread,win32}.c, by making the platform dependence a suffix rather than a prefix of the file name of the implementation, and where src/util/threads.h is the common header that the rest of the source files use. Probably lots more that I could review, but at this point, I'd much rather see smaller patches to individual problems, so that we can quickly commit the no-brainers (such as things that are necessary to fix broken compilation) while still improving on the things that will have review comments (such as adding major features). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Daniel P. Berrange

12:23 p.m.

On Sun, Dec 09, 2012 at 09:17:01PM +0400, Roman Bogorodskiy wrote:

...

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. I won't make you do that work. I'll look at submitting a patch to santize arch handling

...

struct x86_vendor { char *name; 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

...

diff --git a/src/network/default.xml b/src/network/default.xml index 9cfc01e..df34d2e 100644 --- a/src/network/default.xml +++ b/src/network/default.xml @@ -1,6 +1,6 @@ <network> <name>default</name> - <bridge name="virbr0" /> + <bridge name="bridge0" />

NACK to this one. The default bridge name should be kept as 'virbr0' even on BSD, since this is standard libvirt naming convention.

...

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 096000b..db6f401 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -38,6 +38,11 @@ # include <numa.h> #endif +#if defined(__FreeBSD__) +#include <sys/types.h> +#include <sys/sysctl.h> +#endif + #include "c-ctype.h" #include "memory.h" #include "nodeinfo.h" @@ -841,6 +846,24 @@ error: } #endif +#ifdef __FreeBSD__ +static int freebsdCPUCount(void); + +static int +freebsdCPUCount(void) +{ + int ncpu_mib[2] = { CTL_HW, HW_NCPU }; + unsigned long ncpu; + size_t ncpu_len = sizeof(ncpu); + + if (sysctl(ncpu_mib, 2, &ncpu, &ncpu_len, NULL, 0) == -1) { + return -1; + } + + return ncpu; +} +#endif + int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { struct utsname info; @@ -871,6 +894,43 @@ cleanup: VIR_FORCE_FCLOSE(cpuinfo); return ret; } +#elif defined(__FreeBSD__) + { + nodeinfo->nodes = 1; + nodeinfo->sockets = 1; + nodeinfo->cores = 1; + nodeinfo->threads = 1; + + nodeinfo->cpus = freebsdCPUCount();

You're not handling possible '-1' return value

...

+ + unsigned long cpu_freq; + size_t cpu_freq_len = sizeof(cpu_freq); + + if (sysctlbyname("dev.cpu.0.freq", &cpu_freq, &cpu_freq_len, NULL, 0) < 0) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot obtain cpu freq")); + return -1; + } + + nodeinfo->mhz = cpu_freq; + + /* get memory information */ + int mib[2] = { CTL_HW, HW_PHYSMEM }; + unsigned long physmem; + size_t len = sizeof(physmem); + + if (sysctl(mib, 2, &physmem, &len, NULL, 0) == -1) { + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("cannot obtain memory count")); + return -1; + } + + nodeinfo->memory = (unsigned long)(physmem / 1024); + + VIR_DEBUG("memory: %lu", nodeinfo->memory); + + return 0; + } #else /* XXX Solaris will need an impl later if they port QEMU driver */ virReportError(VIR_ERR_NO_SUPPORT, "%s", @@ -1008,6 +1068,8 @@ nodeGetCPUCount(void) VIR_FREE(cpupath); return i; +#elif defined(__FreeBSD__) + return freebsdCPUCount(); #else virReportError(VIR_ERR_NO_SUPPORT, "%s", _("host cpu counting not implemented on this platform"));

I'd ack this part if it was submitted on its own.

...

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 01a1b98..c445e83 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -1623,6 +1623,13 @@ uname_normalize(struct utsname *ut) ut->machine[3] == '6' && ut->machine[4] == '\0') ut->machine[1] = '6'; + +#if defined(__FreeBSD__) + /* We need to map 'amd64' -> 'x86_64' */ + if (strncmp(ut->machine, "amd64", strlen(ut->machine) + 1) == 0) { + strlcpy(ut->machine, "x86_64", sizeof(ut->machine)); + } +#endif /* defined(__FreeBSD__) */

This is a nice example of what i was talking about earlier.

...

int qemuCapsGetDefaultVersion(virCapsPtr caps, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5335dcf..bbbfbb7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -270,11 +270,15 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; } + VIR_WARN("%s, brname = %s, ifname = %s, mac = %s", + __func__, brname, net->ifname, &net->mac); err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, def->uuid, &tapfd, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), tap_create_flags); + VIR_WARN("%s, added ifname %s to bridge %s", __func__, + net->ifname, brname); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0); if (err < 0) { if (template_ifname) @@ -6006,6 +6010,7 @@ qemuBuildCommandLine(virConnectPtr conn, if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || driver->privileged || (!qemuCapsGet(caps, QEMU_CAPS_NETDEV_BRIDGE))) { + VIR_WARN("Before if connect"); int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, caps); if (tapfd < 0)

All bogus warnings left over debugging data i guess.

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8d380a1..b07b185 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -34,7 +34,9 @@ #include <sys/wait.h> #include <arpa/inet.h> #include <sys/utsname.h> +#ifdef HAVE_MNTENT_H #include <mntent.h> +#endif /* HAVE_MNTENT_H */ #include "virterror_internal.h" #include "qemu_conf.h"

I believe this header can be deleted even on Linux, since the code that used it moved into src/util/util.c as virFileFindMountPoint

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e099c5c..4412544 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2133,6 +2133,11 @@ qemuDomainDestroyFlags(virDomainPtr dom, qemuDomainSetFakeReboot(driver, vm, false); + /* We need to prevent monitor EOF callback from doing our work (and sending + * misleading events) while the vm is unlocked inside BeginJob API + */ + priv->beingDestroyed = true; + /* Although qemuProcessStop does this already, there may * be an outstanding job active. We want to make sure we * can kill the process even if a job is active. Killing @@ -2146,11 +2151,6 @@ qemuDomainDestroyFlags(virDomainPtr dom, goto cleanup; } - /* We need to prevent monitor EOF callback from doing our work (and sending - * misleading events) while the vm is unlocked inside BeginJob API - */ - priv->beingDestroyed = true; - if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_DESTROY) < 0) goto cleanup;

Huh, why this change ?

...

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab04599..574caee 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -27,7 +27,15 @@ #include <sys/stat.h> #include <sys/time.h> #include <sys/resource.h> + +#if defined(__linux__) #include <linux/capability.h> +#endif + +#if defined(__FreeBSD__) +#include <sys/param.h> +#include <sys/cpuset.h> +#endif #include "qemu_process.h" #include "qemu_domain.h" @@ -3704,11 +3712,13 @@ int qemuProcessStart(virConnectPtr conn, if (driver->clearEmulatorCapabilities) virCommandClearCaps(cmd); +#if defined(__linux__) /* 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) virCommandAllowCap(cmd, CAP_SYS_RAWIO); } +#endif

Here, you need to report an error with VIR_ERR_CONFIG_UNSUPPORTED if rawio==1, rather than commenting it out entirely.

...

@@ -4132,6 +4142,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualVirtPortProfile(net), driver->stateDir)); VIR_FREE(net->ifname); + } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_BRIDGE) { + ignore_value(virNetDevBridgeRemovePort(virDomainNetGetActualBridgeName(net), + net->ifname)); + ignore_value(virNetDevTapDelete(net->ifname)); } /* release the physical device (or any other resources used by * this interface in the network driver

Hmm, what's this about ? Seems unrelated to BSD to me ?

...

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index a1b64d7..b676436 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1090,8 +1090,7 @@ int virNetSocketGetPort(virNetSocketPtr sock) return port; } - -#ifdef SO_PEERCRED +#if defined(SO_PEERCRED) int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, uid_t *uid, gid_t *gid, @@ -1115,6 +1114,34 @@ int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, virMutexUnlock(&sock->lock); return 0; } +#elif defined(LOCAL_PEERCRED) +int virNetSocketGetUNIXIdentity(virNetSocketPtr sock, + uid_t *uid, + gid_t *gid, + pid_t *pid) +{ + #include <sys/ucred.h>

Header includes should be at the top fo the file, not in the function body.

...

+ + struct xucred cr; + socklen_t cr_len = sizeof(cr); + virMutexLock(&sock->lock); + + if (getsockopt(sock->fd, SOL_SOCKET, LOCAL_PEERCRED, &cr, &cr_len) < 0) { + virReportSystemError(errno, "%s", + _("Failed to get client socket identity")); + virMutexUnlock(&sock->lock); + return -1; + } + + /* *pid = cr.pid; */ + *pid = 0;

I'm a little wary of setting this to 0. This function can be used to do security checks on the client. By setting it to 0, you are effectively saying that the RPC client is running as root, which would give it full privileges. Probably better to set to '-1' (which will be a high numbered unprivileged user).

...

+ *uid = cr.cr_uid; + *gid = cr.cr_gid; + + virMutexUnlock(&sock->lock); + return 0; +}

I'd ACK this with that change

...

diff --git a/src/util/processinfo.c b/src/util/processinfo.c index b1db049..77639d2 100644 --- a/src/util/processinfo.c +++ b/src/util/processinfo.c @@ -168,6 +168,24 @@ realloc: return 0; } +#elif defined(__FreeBSD__) + +int virProcessInfoSetAffinity(pid_t pid, virBitmapPtr map) +{ + return 0; +} + +int virProcessInfoGetAffinity(pid_t pid, virBitmapPtr *map, int maxcpu) +{ + *map = virBitmapNew(maxcpu); + if (!map) { + virReportOOMError(); + return -1; + } + + return 0; +} +

Hmm, this tells the caller that the process is not running on any CPUs. In the absence of a real impl for BSD, I think we'd probably be safer to set the mask to all-1s.

...

#else /* HAVE_SCHED_GETAFFINITY */ int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, @@ -179,8 +197,8 @@ int virProcessInfoSetAffinity(pid_t pid ATTRIBUTE_UNUSED, } int virProcessInfoGetAffinity(pid_t pid ATTRIBUTE_UNUSED, - virBitmapPtr *map ATTRIBUTE_UNUSED, - int maxcpu ATTRIBUTE_UNUSED) + virBitmapPtr *map ATTRIBUTE_UNUSED, + int maxcpu ATTRIBUTE_UNUSED)

Bogus whitespace change.

...

{ virReportSystemError(ENOSYS, "%s", _("Process CPU affinity is not supported on this platform")); diff --git a/src/util/virinitctl.c b/src/util/virinitctl.c index cdd3dc0..54030ee 100644 --- a/src/util/virinitctl.c +++ b/src/util/virinitctl.c @@ -101,7 +101,9 @@ struct virInitctlRequest { } i; }; +#if !defined(__FreeBSD__) verify(sizeof(struct virInitctlRequest) == 384); +#endif

Bogus. Can you try and find out what part of the struct changed size, so we can figure out the right fix.

...

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index c345013..575d02d 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -32,9 +32,11 @@ #include "logging.h" #include <sys/ioctl.h> +#include <sys/socket.h> #include <net/if.h> #include <fcntl.h> + #ifdef __linux__ # include <linux/sockios.h> # include <linux/if_vlan.h> @@ -42,6 +44,10 @@ # undef HAVE_STRUCT_IFREQ #endif +#ifdef __FreeBSD__ +# include <sys/sockio.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE #if defined(HAVE_STRUCT_IFREQ) @@ -119,6 +125,35 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevExists(const char *ifname) +{ + int s; + int ret = -1; + + struct ifreq ifr; + memset(&ifr, 0, sizeof(ifr)); + strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name)); + + VIR_WARN("%s, ifname = %s", __func__, ifname);

Use DEBUG not WARN for ad-hoc debugging.

...

+ + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + + if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0) { + if (errno == ENXIO) + ret = 0; + else + virReportSystemError(errno, + _("Unable to check interface flags for %s"), ifname); + goto cleanup; + } + + ret = 1; + +cleanup: + close(s); + return ret; +} #else int virNetDevExists(const char *ifname) { @@ -173,6 +208,51 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +#include <net/if_dl.h> + +int virNetDevSetMAC(const char *ifname, + const virMacAddrPtr macaddr) +{ + struct ifreq ifr; + struct sockaddr_dl sdl; + uint8_t mac[19]; + char *macstr; + int s; + int ret = -1; + + macstr = malloc(sizeof(char) * VIR_MAC_STRING_BUFLEN);

Direct use of 'malloc' is forbidden in libvirt code. Use VIR_ALLOC_N instead - see HACKING for further help on the APIs.

...

+ virMacAddrFormat(&macaddr, macstr); + + VIR_WARN("%s: setting mac addr on %s to %s", + __func__, ifname, macstr); + + strncpy(ifr.ifr_name, ifname, IFNAMSIZ); + memset(mac, 0, sizeof(mac)); + mac[0] = ':'; + strncpy(mac + 1, macstr, strlen(macstr));

'strncpy' is forbidden by syntax-check rules too. See virStrncpy instead.

...

+ sdl.sdl_len = sizeof(sdl); + link_addr(mac, &sdl); + + bcopy(sdl.sdl_data, ifr.ifr_addr.sa_data, 6); + ifr.ifr_addr.sa_len = 6; + + s = socket(AF_LOCAL, SOCK_DGRAM, 0); + + if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface MAC on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; +cleanup: + free(macstr); + close(s);

close is forbidden - use VIR_FORCE_CLOSE. NB, it is preferrable to run 'make syntax-check' before submitting any patches to identify problems like this. I know this probably isn't possible on a BSD host, but you can at least run it on a Linux host - make syntax-check doesn't required that the BSD code is compiled - it is static analysis directly on the source.

...

+ + return ret; +} #else int virNetDevSetMAC(const char *ifname, const virMacAddrPtr macaddr ATTRIBUTE_UNUSED) @@ -350,6 +430,34 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevGetMTU(const char *ifname) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { + return -1;

Missing virReportSystemError()

...

+ } + + ifr.ifr_addr.sa_family = AF_INET; + strcpy(ifr.ifr_name, ifname); + if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + goto cleanup; + } + + ret = ifr.ifr_mtu; + + ret = 0; + +cleanup: + close(s); + return ret; +} #else int virNetDevGetMTU(const char *ifname) { @@ -396,6 +504,34 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetMTU(const char *ifname, int mtu) +{ + int s; + int ret; + struct ifreq ifr; + + if ((s = socket(AF_INET, SOCK_DGRAM, 0)) < 0) { + return -1;

Missing virReportSystemError

...

+ } + + ifr.ifr_addr.sa_family = AF_INET; + strcpy(ifr.ifr_name, ifname); + ifr.ifr_mtu = mtu; + + if (ioctl(s, SIOCGIFMTU, (caddr_t)&ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface MTU on '%s'"), + ifname); + goto cleanup; + } + + ret = 0; + +cleanup: + close(s); + return ret; +} #else int virNetDevSetMTU(const char *ifname, int mtu ATTRIBUTE_UNUSED) { @@ -558,6 +694,50 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevSetOnline(const char *ifname, + bool online) +{ + int s; + int ret = -1; + int ifflags; + + VIR_WARN("%s, ifname = %s", __func__, ifname); + + struct ifreq ifr; + memset(&ifr, 0, sizeof(ifr)); + strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));

Use virStrncpy instead

...

+ + s = socket(AF_LOCAL, SOCK_DGRAM, 0);

Missing check for failure

...

+ + if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface flags on '%s'"), + ifname); + goto cleanup; + } + + if (online) + ifflags = ifr.ifr_flags | IFF_UP; + else + ifflags = ifr.ifr_flags & ~IFF_UP; + + if (ifr.ifr_flags != ifflags) { + ifr.ifr_flags = ifflags; + if (ioctl(s, SIOCSIFFLAGS, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot set interface flags on '%s'"), + ifname); + goto cleanup; + } + } + + ret = 0; + +cleanup: + close(s); + return ret; +} #else int virNetDevSetOnline(const char *ifname, bool online ATTRIBUTE_UNUSED) @@ -604,6 +784,36 @@ cleanup: VIR_FORCE_CLOSE(fd); return ret; } +#elif defined(__FreeBSD__) +int virNetDevIsOnline(const char *ifname, + bool *online) +{ + int s; + int ret = -1; + + struct ifreq ifr; + memset(&ifr, 0, sizeof(ifr)); + strlcpy(ifr.ifr_name, ifname, sizeof(ifr.ifr_name));

See earlier > + > + VIR_WARN("%s, ifname = %s", __func__, ifname);

...

+ + s = socket(AF_LOCAL, SOCK_DGRAM, 0);

See earlier

...

+ + if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0) { + virReportSystemError(errno, + _("Cannot get interface flags on '%s'"), + ifname); + goto cleanup; + } + + *online = (ifr.ifr_flags & IFF_UP) ? true : false; + ret = 0; + +cleanup: + close(s); + return ret; + +} #else int virNetDevIsOnline(const char *ifname, bool *online ATTRIBUTE_UNUSED) @@ -750,12 +960,20 @@ int virNetDevSetIPv4Address(const char *ifname, !(bcaststr = virSocketAddrFormat(&broadcast)))) { goto cleanup; } +#if defined(__FreeBSD__) + cmd = virCommandNew(IFCONFIG_PATH); + virCommandAddArgList(cmd, ifname, "inet", NULL); + virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); + if (bcaststr) + virCommandAddArgList(cmd, "broadcast", bcaststr, NULL); +#else cmd = virCommandNew(IP_PATH); virCommandAddArgList(cmd, "addr", "add", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); if (bcaststr) virCommandAddArgList(cmd, "broadcast", bcaststr, NULL); virCommandAddArgList(cmd, "dev", ifname, NULL); +#endif if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -789,10 +1007,17 @@ int virNetDevClearIPv4Address(const char *ifname, if (!(addrstr = virSocketAddrFormat(addr))) goto cleanup; +#if defined(__FreeBSD__) + cmd = virCommandNew(IFCONFIG_PATH); + virCommandAddArgList(cmd, ifname, "inet", NULL); + virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); + virCommandAddArgList(cmd, "-alias", NULL); +#else cmd = virCommandNew(IP_PATH); virCommandAddArgList(cmd, "addr", "del", NULL); virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); virCommandAddArgList(cmd, "dev", ifname, NULL); +#endif if (virCommandRun(cmd, NULL) < 0) goto cleanup;

The changes in this file mostly seem reasonable, assuming the fixes are made Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Jiri Denemark

2:11 p.m.

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

Roman Bogorodskiy

Tuesday, 11 December Tue, 11 Dec

10:54 a.m.

Daniel P. Berrange wrote:

...

Hi, Thanks for the valuable comments, I will definitely consider them when breaking the patch into smaller chunks. As for you question on tap devices clean-up: on FreeBSD tap interfaces doesn't get automatically destroyed when they stop being hold by a process, so one would have to destroy them manually. Roman Bogorodskiy

4367

days inactive

4369

days old

devel@lists.libvirt.org

Manage subscription

4 comments

4 participants

tags (0)

participants (4)

Daniel P. Berrange
Eric Blake
Jiri Denemark
Roman Bogorodskiy

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH v0] qemu driver FreeBSD support