On Thu, Jan 23, 2014 at 11:05:26PM +0400, Roman Bogorodskiy wrote:
> At this point it has a limited functionality and is highly
> experimental. Supported domain operations are:
>
> * define
> * start
> * destroy
> * dumpxml
> * dominfo
>
> It's only possible to have only one disk device and only one
> network, which should be of type bridge.
> diff --git a/configure.ac b/configure.ac
> index 3a70375..bfbd3a8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -524,6 +524,10 @@ AC_ARG_WITH([parallels],
> [AS_HELP_STRING([--with-parallels],
> [add Parallels Cloud Server support @<:@default=check@:>@])])
> m4_divert_text([DEFAULTS], [with_parallels=check])
> +AC_ARG_WITH([bhyve],
> + [AS_HELP_STRING([--with-bhyve],
> + [add BHyVe support @<:@default=check@:>@])])
> +m4_divert_text([DEFAULTS], [with_bhyve=check])
I think it is probably possible to move this into the
LIBVIRT_DRIVER_CHECK_BHYVE macro too. If that doesn't
work then just create a LIBVIRT_DRIVER_ARG_BHYVE macro
for it. Then we have everything in the isolated .m4 file
> AC_ARG_WITH([test],
> [AS_HELP_STRING([--with-test],
> [add test driver support @<:@default=yes@:>@])])
> @@ -1031,6 +1035,12 @@ fi
> AM_CONDITIONAL([WITH_PARALLELS], [test "$with_parallels" = "yes"])
>
> dnl
> +dnl Checks for bhyve driver
> +dnl
> +
> +LIBVIRT_DRIVER_CHECK_BHYVE
> +
> +dnl
> dnl check for shell that understands <> redirection without truncation,
> dnl needed by src/qemu/qemu_monitor_{text,json}.c.
> dnl
> @@ -2677,6 +2687,7 @@ AC_MSG_NOTICE([ PHYP: $with_phyp])
> AC_MSG_NOTICE([ ESX: $with_esx])
> AC_MSG_NOTICE([ Hyper-V: $with_hyperv])
> AC_MSG_NOTICE([Parallels: $with_parallels])
> +LIBIVRT_DRIVER_RESULT_BHYVE
> AC_MSG_NOTICE([ Test: $with_test])
> AC_MSG_NOTICE([ Remote: $with_remote])
> AC_MSG_NOTICE([ Network: $with_network])
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8f77658..6b95f64 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -515,6 +515,7 @@ DRIVER_SOURCE_FILES = \
> $(NULL)
>
> STATEFUL_DRIVER_SOURCE_FILES = \
> + $(BHYVE_DRIVER_SOURCES) \
> $(INTERFACE_DRIVER_SOURCES) \
> $(LIBXL_DRIVER_SOURCES) \
> $(LXC_DRIVER_SOURCES) \
Nit-pick - inconsistent indentation - the other lines use
tabs, but you've used spaces. Although we dislike tabs,
just follow existing practice in this file
> @@ -772,6 +773,15 @@ PARALLELS_DRIVER_SOURCES = \
> parallels/parallels_storage.c \
> parallels/parallels_network.c
>
> +BHYVE_DRIVER_SOURCES = \
> + bhyve/bhyve_command.c \
> + bhyve/bhyve_command.h \
> + bhyve/bhyve_driver.h \
> + bhyve/bhyve_driver.c \
> + bhyve/bhyve_process.c \
> + bhyve/bhyve_process.h \
> + bhyve/bhyve_utils.h
Same indentation note here.
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> new file mode 100644
> index 0000000..f2988cf
> --- /dev/null
> +++ b/src/bhyve/bhyve_command.c
> @@ -0,0 +1,269 @@
> +static char*
> +virTapGetRealDeviceName(char *name)
Can you aim to use 'virBhyve' as the pefix for absolutely
every function and struct you define in src/bhyve/ files.
We've not been all that good at this in other virt drivers
but its the rough goal we're aiming for is to have all
function prefixes match filename prefix.
> +{
> + /* This is an ugly hack, because if we rename
> + * tap device to vnet%d, its device name will be
> + * still /dev/tap%d, and bhyve tries too open /dev/tap%d,
> + * so we have to find the real name
> + */
> + char *ret = NULL;
> + struct dirent *dp;
> + char *devpath = NULL;
> + int fd;
> +
> + DIR* dirp = opendir("/dev");
'*' should associate with the variable rather than the type.
> + if (dirp == NULL) {
> + return NULL;
> + }
virReportSystemError() when this fails.
> +
> + while ((dp = readdir(dirp)) != NULL) {
> + if (STRPREFIX(dp->d_name, "tap")) {
> + struct ifreq ifr;
> + if (virAsprintf(&devpath, "/dev/%s", dp->d_name) < 0) {
> + goto cleanup;
> + }
> + if ((fd = open(devpath, O_RDWR)) < 0)
> + goto cleanup;
virReportSystemError()
> +
> + if (ioctl(fd, TAPGIFNAME, (void *)&ifr) < 0)
> + goto cleanup;
virReportSystemError()
> +
> + if (STREQ(name, ifr.ifr_name)) {
> + /* we can ignore the return value
> + * because we still have nothing
> + * to do but return;
> + */
> + ignore_value(VIR_STRDUP(ret, dp->d_name));
> + goto cleanup;
> + }
> +
> + VIR_FREE(devpath);
> + VIR_FORCE_CLOSE(fd);
> + }
> + }
> +
> +cleanup:
> + VIR_FREE(devpath);
> + VIR_FORCE_CLOSE(fd);
> + closedir(dirp);
> + return ret;
> +}
> +
> +static int
> +bhyveBuildNetArgStr(const virDomainDef *def, virCommandPtr cmd)
> +{
> + virDomainNetDefPtr net = NULL;
> + char *brname = NULL;
> + char *realifname = NULL;
> + int *tapfd = NULL;
> +
> + if (def->nnets != 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain should have one and only one net defined"));
> + return -1;
> + }
> +
> + net = def->nets[0];
> +
> + if (net != NULL) {
> + int actualType = virDomainNetGetActualType(net);
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> + if (VIR_STRDUP(brname, virDomainNetGetActualBridgeName(net)) < 0)
> + return -1L;
> + } else {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Network type %d is not supported"),
> + virDomainNetGetActualType(net));
> + return -1;
> + }
> +
> + if (!net->ifname ||
> + STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
> + strchr(net->ifname, '%')) {
> + VIR_FREE(net->ifname);
> + if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_PREFIX "%d") < 0) {
> + VIR_FREE(brname);
> + return -1;
> + }
> + }
> +
> + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> + def->uuid, tapfd, 1,
> + virDomainNetGetActualVirtPortProfile(net),
> + virDomainNetGetActualVlan(net),
> + VIR_NETDEV_TAP_CREATE_IFUP | VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> + VIR_FREE(net->ifname);
> + VIR_FREE(brname);
> + return -1;
> + }
> + }
> +
> + realifname = virTapGetRealDeviceName(net->ifname);
> +
> + if (realifname == NULL) {
> + VIR_FREE(net->ifname);
> + VIR_FREE(brname);
> + return -1;
> + }
> +
> + VIR_INFO("%s -> %s", net->ifname, realifname);
s/INFO/DEBUG/
> + /* hack on top of other hack: we need to set
> + * interface to 'UP' again after re-opening to find its
> + * name
> + */
> + if (virNetDevSetOnline(net->ifname, true) != 0) {
> + VIR_FREE(net->ifname);
> + VIR_FREE(brname);
> + return -1;
> + }
> +
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArg(cmd, "0:0,hostbridge");
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArgFormat(cmd, "1:0,virtio-net,%s", realifname);
> +
> + return 0;
> +}
> +virCommandPtr
> +virBhyveProcessBuildBhyveCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> +{
> + /*
> + * /usr/sbin/bhyve -c 2 -m 256 -AI -H -P \
> + * -s 0:0,hostbridge \
> + * -s 1:0,virtio-net,tap0 \
> + * -s 2:0,ahci-hd,${IMG} \
> + * -S 31,uart,stdio \
> + * vm0
> + */
> + virCommandPtr cmd = NULL;
> + cmd = virCommandNew(BHYVE);
simpler to merge these onto 1 line.
> +
> + /* CPUs */
> + virCommandAddArg(cmd, "-c");
> + virCommandAddArgFormat(cmd, "%d", vm->def->vcpus);
> +
> + /* Memory */
> + virCommandAddArg(cmd, "-m");
> + virCommandAddArgFormat(cmd, "%llu",
> + VIR_DIV_UP(vm->def->mem.max_balloon, 1024));
> +
> + /* Options */
> + virCommandAddArg(cmd, "-A"); /* Create an ACPI table */
You can look at the 'acpi' features flag to turn this on/off
> + virCommandAddArg(cmd, "-I"); /* Present ioapic to the guest */
Likewise you can look at the 'apic' features flag to turn this on/off
> + virCommandAddArg(cmd, "-H"); /* vmexit from guest on hlt */
> + virCommandAddArg(cmd, "-P"); /* vmexit from guest on pause */
What's the functional effect of having these set, or not ?
> +
> + /* Devices */
> + if (bhyveBuildNetArgStr(vm->def, cmd) < 0)
> + goto error;
> + if (bhyveBuildDiskArgStr(vm->def, cmd) < 0)
> + goto error;
> + virCommandAddArg(cmd, "-S");
> + virCommandAddArg(cmd, "31,uart");
> + virCommandAddArg(cmd, vm->def->name);
> +
> + return cmd;
> +
> +error:
> + virCommandFree(cmd);
> + return NULL;
> +}
> +
> +virCommandPtr
> +virBhyveProcessBuildDestroyCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> +{
> + virCommandPtr cmd = virCommandNew(BHYVECTL);
> +
> + virCommandAddArg(cmd, "--destroy");
> + virCommandAddArgPair(cmd, "--vm", vm->def->name);
> +
> + return cmd;
> +}
> +
> +virCommandPtr
> +virBhyveProcessBuildLoadCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> +{
> + virCommandPtr cmd;
> + virDomainDiskDefPtr disk = vm->def->disks[0];
> +
> + cmd = virCommandNew(BHYVELOAD);
> +
> + /* Memory */
> + virCommandAddArg(cmd, "-m");
> + virCommandAddArgFormat(cmd, "%llu",
> + VIR_DIV_UP(vm->def->mem.max_balloon, 1024));
> +
> + /* Image path */
> + virCommandAddArg(cmd, "-d");
> + virCommandAddArgFormat(cmd, disk->src);
> +
> + /* VM name */
> + virCommandAddArg(cmd, vm->def->name);
> +
> + return cmd;
> +}
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> new file mode 100644
> index 0000000..5ddb26a
> --- /dev/null
> +++ b/src/bhyve/bhyve_driver.c
> diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
> new file mode 100644
> index 0000000..95553d7
> --- /dev/null
> +++ b/src/bhyve/bhyve_process.c
> @@ -0,0 +1,205 @@
> +/*
> + * bhyve_process.c: bhyve process management
> + *
> + * Copyright (C) 2013 Roman Bogorodskiy
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include <config.h>
> +
> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <dirent.h>
> +#include <sys/ioctl.h>
> +#include <net/if.h>
> +#include <net/if_tap.h>
> +
> +#include "bhyve_process.h"
> +#include "bhyve_command.h"
> +#include "datatypes.h"
> +#include "virerror.h"
> +#include "virlog.h"
> +#include "virfile.h"
> +#include "viralloc.h"
> +#include "vircommand.h"
> +#include "virstring.h"
> +#include "virpidfile.h"
> +#include "virprocess.h"
> +#include "virnetdev.h"
> +#include "virnetdevbridge.h"
> +#include "virnetdevtap.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_BHYVE
> +
> +int
> +virBhyveProcessStart(virConnectPtr conn,
> + bhyveConnPtr driver,
> + virDomainObjPtr vm,
> + virDomainRunningReason reason ATTRIBUTE_UNUSED)
> +{
> + char *logfile = NULL;
> + int logfd = -1;
> + off_t pos = -1;
> + char ebuf[1024];
> + virCommandPtr cmd = NULL;
> + bhyveConnPtr privconn = conn->privateData;
> + int ret = -1, status;
> +
> + if (vm->def->ndisks != 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Domain should have one and only one disk defined"));
> + return -1;
> + }
I'd suggest that can live in just the virBhyveProcessBuildBhyveCmd
method
> +int
> +virBhyveProcessStop(bhyveConnPtr driver,
> + virDomainObjPtr vm,
> + virDomainShutoffReason reason ATTRIBUTE_UNUSED)
> +{
> + int i;
> + int ret = -1;
> + int status;
> + virCommandPtr cmd = NULL;
> +
Still want to sanity check with virDomainObjIsActive() before
proceeeding there
> + /* First, try to kill 'bhyve' process */
> + if (virProcessKillPainfully(vm->pid, true) != 0)
> + VIR_WARN("Failed to gracefully stop bhyve VM '%s' (pid: %llu)",
> + vm->def->name,
> + (unsigned long long)vm->pid);
> +
> + for (i = 0; i < vm->def->nnets; i++) {
> + virDomainNetDefPtr net = vm->def->nets[i];
> + int actualType = virDomainNetGetActualType(net);
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> + ignore_value(virNetDevBridgeRemovePort(
> + virDomainNetGetActualBridgeName(net),
> + net->ifname));
> + ignore_value(virNetDevTapDelete(net->ifname));
> + }
> + }
> +
> + /* No matter if shutdown was successful or not, we
> + * need to unload the VM */
> + if (!(cmd = virBhyveProcessBuildDestroyCmd(driver, vm)))
> + goto cleanup;
> +
> + if (virCommandRun(cmd, &status) < 0)
> + goto cleanup;
> +
> + if (status != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Guest failed to stop: %d"), status);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + virCommandFree(cmd);
> + return ret;
> +}
> +
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list