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. 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 :|