On Thu, Jan 02, 2014 at 12:27:52PM +0400, Roman Bogorodskiy wrote:
> At this point it has a limited functionality and is highly
> experimental. Supported domain operations are:
> * define
> * start
> * destroy
>
> It's only possible to have only one disk device and only one
> network, which should be of type bridge.
Can you tell us a little bit about how bhyve works from an
architectural POV ? Is it similar in design to KVM where
you basically just have a QEMU like emulator in userspace
for all management tasks ? Or are there further command
line utilities for management ?
If multiple user accounts are using bhyve are they all
completely in their own isolated world (eg like QEMU which
lets us have a per-user view qemu:///session) or is the
list of VMs always global (like Xen) ?
Is this driver intending to be interoperating with other
bhyve management tools, or is it standalone ?
> ---
> configure.ac | 37 ++++
> daemon/libvirtd.c | 9 +
> include/libvirt/virterror.h | 1 +
> po/POTFILES.in | 2 +
> src/Makefile.am | 35 +++
> src/bhyve/bhyve_driver.c | 514 ++++++++++++++++++++++++++++++++++++++++++++
> src/bhyve/bhyve_driver.h | 28 +++
> src/bhyve/bhyve_process.c | 393 +++++++++++++++++++++++++++++++++
> src/bhyve/bhyve_process.h | 36 ++++
> src/bhyve/bhyve_utils.h | 48 +++++
> src/conf/domain_conf.c | 3 +-
> src/conf/domain_conf.h | 1 +
> src/driver.h | 1 +
> src/libvirt.c | 3 +
> src/util/virerror.c | 1 +
> 15 files changed, 1111 insertions(+), 1 deletion(-)
> create mode 100644 src/bhyve/bhyve_driver.c
> create mode 100644 src/bhyve/bhyve_driver.h
> create mode 100644 src/bhyve/bhyve_process.c
> create mode 100644 src/bhyve/bhyve_process.h
> create mode 100644 src/bhyve/bhyve_utils.h
>
> diff --git a/configure.ac b/configure.ac
> index 2622dfd..edfe7d4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -504,6 +504,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])
> AC_ARG_WITH([test],
> [AS_HELP_STRING([--with-test],
> [add test driver support @<:@default=yes@:>@])])
> @@ -1011,6 +1015,38 @@ fi
> AM_CONDITIONAL([WITH_PARALLELS], [test "$with_parallels" = "yes"])
>
> dnl
> +dnl Checks for bhyve driver
> +dnl
> +
> +if test "$with_bhyve" != "no"; then
> + AC_PATH_PROG([BHYVE], [bhyve], [], [$PATH:/usr/sbin])
> + AC_PATH_PROG([BHYVECTL], [bhyvectl], [$PATH:/usr/sbin])
> + AC_PATH_PROG([BHYVELOAD], [bhyveload], [$PATH:/usr/sbin/])
> +
> + if test -z "$BHYVE" || test -z "$BHYVECTL" \
> + test -z "$BHYVELOAD" || test "$with_freebsd" = "no"; then
> + if test "$with_bhyve" = "check"; then
> + with_bhyve="no"
> + else
> + AC_MSG_ERROR([The bhyve driver cannot be enabled])
> + fi
> + else
> + with_bhyve="yes"
> + fi
> +fi
> +
> +if test "$with_bhyve" = "yes"; then
> + AC_DEFINE_UNQUOTED([WITH_BHYVE], 1, [whether bhyve driver is enabled])
> + AC_DEFINE_UNQUOTED([BHYVE], ["$BHYVE"],
> + [Location of the bhyve tool])
> + AC_DEFINE_UNQUOTED([BHYVECTL], ["$BHYVECTL"],
> + [Location of the bhyvectl tool])
> + AC_DEFINE_UNQUOTED([BHYVELOAD], ["$BHYVELOAD"],
> + [Location of the bhyveload tool])
> +fi
> +AM_CONDITIONAL([WITH_BHYVE], [test "$with_bhyve" = "yes"])
In an attempt to start the trend of splitting up configure.ac can you
put all this code in a m4/virt-driver-bhyve.m4 file inside a macro
and then in configure.ac you can just invoke:
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
> @@ -2582,6 +2618,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])
> +AC_MSG_NOTICE([ Bhyve: $with_bhyve])
> AC_MSG_NOTICE([ Test: $with_test])
> AC_MSG_NOTICE([ Remote: $with_remote])
> AC_MSG_NOTICE([ Network: $with_network])
And here this can also be in a macro which you can invoke as
LIBIVRT_DRIVER_RESULT_BHYVE
> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> new file mode 100644
> index 0000000..ba62d1a
> --- /dev/null
> +++ b/src/bhyve/bhyve_driver.c
> +bhyveConnPtr bhyve_driver = NULL;
> +
> +void
> +bhyveDriverLock(bhyveConnPtr driver)
> +{
> + virMutexLock(&driver->lock);
> +}
> +
> +void
> +bhyveDriverUnlock(bhyveConnPtr driver)
> +{
> + virMutexUnlock(&driver->lock);
> +}
I don't know if you've looked at the QEMU / LXC drivers in any great
detail, but I'd suggest you try to follow the way they do locking. We
originally had alot of global driver locking but it is a horibble
scalability bottleneck, so we've made the locking very fine grained.
This was somewhat tedious, so I'd suggest you try to have the fine
grained locking right from the start to make your lives easier in the
long term. In particular look at the comments in src/qemu/qemu_conf.h
about locking.
> +
> +static virCapsPtr
> +bhyveBuildCapabilities(void)
> +{
> + virCapsPtr caps;
> + virCapsGuestPtr guest;
> +
> + if ((caps = virCapabilitiesNew(virArchFromHost(),
> + 0, 0)) == NULL)
> + return NULL;
> +
> + if ((guest = virCapabilitiesAddGuest(caps, "hvm",
> + VIR_ARCH_X86_64,
> + "bhyve",
> + NULL, 0, NULL)) == NULL)
> + goto error;
> +
> + if (virCapabilitiesAddGuestDomain(guest,
> + "bhyve", NULL, NULL, 0, NULL) == NULL)
> + goto error;
> +
> + return caps;
> +
> +error:
> + virObjectUnref(caps);
> + return NULL;
> +}
> +
> +static char *
> +bhyveConnectGetCapabilities(virConnectPtr conn)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + char *xml;
> +
> + bhyveDriverLock(privconn);
> + if ((xml = virCapabilitiesFormatXML(privconn->caps)) == NULL)
> + virReportOOMError();
> + bhyveDriverUnlock(privconn);
> +
> + return xml;
> +}
> +
> +static virDomainObjPtr
> +bhyveDomObjFromDomain(virDomainPtr domain)
> +{
> + virDomainObjPtr vm;
> + bhyveConnPtr privconn = domain->conn->privateData;
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> +
> + vm = virDomainObjListFindByUUID(privconn->domains, domain->uuid);
> + if (!vm) {
> + virUUIDFormat(domain->uuid, uuidstr);
> + virReportError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching uuid '%s' (%s)"),
> + uuidstr, domain->name);
> + return NULL;
> + }
> +
> + return vm;
> +}
> +
> +static virDrvOpenStatus
> +bhyveConnectOpen(virConnectPtr conn,
> + virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> + unsigned int flags)
> +{
> + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
> +
> + if (!conn->uri)
> + return VIR_DRV_OPEN_DECLINED;
> +
> + if (!conn->uri->scheme || STRNEQ(conn->uri->scheme, "bhyve"))
> + return VIR_DRV_OPEN_DECLINED;
> +
> + if (conn->uri->server)
> + return VIR_DRV_OPEN_DECLINED;
> +
> + if (!STREQ_NULLABLE(conn->uri->path, "/system")) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unexpected bhyve URI path '%s', try bhyve:///system"),
> + conn->uri->path);
> + return VIR_DRV_OPEN_ERROR;
> + }
This URI choice implicitly says the answer to my question earlier is that bhyve
only provides a single world view of VMs, no per-user account view.
> +static virDomainPtr
> +bhyveDomainDefineXML(virConnectPtr conn, const char *xml)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + virDomainPtr dom = NULL;
> + virDomainDefPtr def = NULL;
> + virDomainDefPtr oldDef = NULL;
> + virDomainObjPtr vm = NULL;
> +
> + bhyveDriverLock(privconn);
This is something you'd want to remove and rely on the fact that
virDomainObjListAdd does locking internally to protect againast
dups.
> +
> + if ((def = virDomainDefParseString(xml, privconn->caps, privconn->xmlopt,
> + 1 << VIR_DOMAIN_VIRT_BHYVE,
> + VIR_DOMAIN_XML_INACTIVE)) == NULL) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Can't parse XML desc"));
> + goto cleanup;
> + }
> +
> + if (!(vm = virDomainObjListAdd(privconn->domains, def,
> + privconn->xmlopt,
> + 0, &oldDef)))
> + goto cleanup;
> +
> + VIR_INFO("Creating domain '%s'", vm->def->name);
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> + if (dom) dom->id = vm->def->id;
> +
> + if (virDomainSaveConfig(BHYVE_CONFIG_DIR, vm->def) < 0) {
> + goto cleanup;
> + }
> +
> +cleanup:
> + virDomainDefFree(def);
> + if (vm)
> + virObjectUnlock(vm);
> +
> + bhyveDriverUnlock(privconn);
> + return dom;
> +}
> +
> +static int
> +bhyveConnectListDomains(virConnectPtr conn, int *ids, int maxids)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + int n;
> +
> + n = virDomainObjListGetActiveIDs(privconn->domains, ids, maxids,
> + NULL, NULL);
> +
> + return n;
> +}
> +
> +static int
> +bhyveConnectNumOfDomains(virConnectPtr conn)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + int count;
> +
> + count = virDomainObjListNumOfDomains(privconn->domains, true,
> + NULL, NULL);
> +
> + return count;
> +}
> +
> +static int
> +bhyveConnectListDefinedDomains(virConnectPtr conn, char **const names,
> + int maxnames)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + int n;
> +
> + memset(names, 0, sizeof(*names) * maxnames);
> + n = virDomainObjListGetInactiveNames(privconn->domains, names,
> + maxnames, NULL, NULL);
> +
> + return n;
> +}
> +
> +static int
> +bhyveConnectNumOfDefinedDomains(virConnectPtr conn)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + int count;
> +
> + count = virDomainObjListNumOfDomains(privconn->domains, false,
> + NULL, NULL);
> +
> + return count;
> +}
> +
> +static int
> +bhyveConnectListAllDomains(virConnectPtr conn,
> + virDomainPtr **domains,
> + unsigned int flags)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + int ret = -1;
> +
> + virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ALL, -1);
> +
> + ret = virDomainObjListExport(privconn->domains, conn, domains,
> + NULL, flags);
> +
> + return ret;
> +}
> +
> +static virDomainPtr
> +bhyveDomainLookupByUUID(virConnectPtr conn,
> + const unsigned char *uuid)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + virDomainObjPtr vm;
> + virDomainPtr dom = NULL;
> +
> + vm = virDomainObjListFindByUUID(privconn->domains, uuid);
> +
> + if (!vm) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virUUIDFormat(uuid, uuidstr);
> + virReportError(VIR_ERR_NO_DOMAIN,
> + _("No domain with matching uuid '%s'"), uuidstr);
> + goto cleanup;
> + }
> +
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> + if (dom)
> + dom->id = vm->def->id;
> +
> +cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return dom;
> +}
> +
> +static virDomainPtr bhyveDomainLookupByName(virConnectPtr conn,
> + const char *name)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + virDomainObjPtr vm;
> + virDomainPtr dom = NULL;
> +
> + vm = virDomainObjListFindByName(privconn->domains, name);
> +
> + if (!vm) {
> + virReportError(VIR_ERR_NO_DOMAIN,
> + _("no domain with matching name '%s'"), name);
> + goto cleanup;
> + }
> + dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
> + if (dom)
> + dom->id = vm->def->id;
> +
> +cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return dom;
> +}
> +
> +static int
> +bhyveDomainCreate(virDomainPtr dom)
> +{
> + bhyveConnPtr privconn = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> +
> + if (!(vm = bhyveDomObjFromDomain(dom)))
> + goto cleanup;
> +
> + if (virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + "%s", _("Domain is already running"));
> + goto cleanup;
> + }
> +
> + ret = virBhyveProcessStart(dom->conn, privconn, vm,
> + VIR_DOMAIN_RUNNING_BOOTED);
> +
> +cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return ret;
> +}
> +
> +static int
> +bhyveDomainDestroy(virDomainPtr dom)
> +{
> + bhyveConnPtr privconn = dom->conn->privateData;
> + virDomainObjPtr vm;
> + int ret = -1;
> +
> + if (!(vm = bhyveDomObjFromDomain(dom)))
> + goto cleanup;
> +
> + ret = virBhyveProcessStop(privconn, vm, VIR_DOMAIN_SHUTOFF_DESTROYED);
> +
> +cleanup:
> + if (vm)
> + virObjectUnlock(vm);
> + return ret;
> +}
As a general comment you're going to need to add calls to the vir***EnsureACL()
methods into all these APIs.
If you add your sources to STATEFUL_DRIVER_SOURCE_FILES in Makefile.am then
'make check' should warn you about this.
> diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
> new file mode 100644
> index 0000000..d481d58
> --- /dev/null
> +++ b/src/bhyve/bhyve_process.c
> +
> +#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 "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
> +
> +static char*
> +virTapGetRealDeviceName(char *name)
> +{
> + /* 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");
> + if (dirp == NULL) {
> + return NULL;
> + }
> +
> + 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;
> +
> + if (ioctl(fd, TAPGIFNAME, (void *)&ifr) < 0)
> + goto cleanup;
> +
> + 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;
> +}
In the various places where you return NULL I think you'd want to
invoke virReportError or virReportSystemError if an errno is relevant.
> +
> +static virCommandPtr
> +virBhyveProcessBuildDestroyCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> +{
> + virCommandPtr cmd = virCommandNew(BHYVECTL);
> +
> + virCommandAddArg(cmd, "--destroy");
> + virCommandAddArgPair(cmd, "--vm", vm->def->name);
> +
> + return cmd;
> +}
> +
> +static 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;
> +}
> +
> +static 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;
> + virDomainDiskDefPtr disk = vm->def->disks[0];
> + virDomainNetDefPtr net = NULL;
> + char *brname = NULL;
> + char *realifname = NULL;
> + int *tapfd = NULL;
> +
> + if (vm->def->nnets > 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Domain should have one and only one disk defined"));
> + return NULL;
> + } else if (vm->def->nnets == 1)
> + net = vm->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 NULL;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Network type %d is not supported"),
> + virDomainNetGetActualType(net));
> + return NULL;
> + }
> +
> + 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 NULL;
> + }
> + }
> +
> + if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> + vm->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 NULL;
> + }
> + }
> +
> + realifname = virTapGetRealDeviceName(net->ifname);
> +
> + if (realifname == NULL) {
> + VIR_FREE(net->ifname);
> + VIR_FREE(brname);
> + return NULL;
> + }
> +
> + VIR_INFO("%s -> %s", net->ifname, realifname);
> + /* 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 NULL;
> + }
> +
> + cmd = virCommandNew(BHYVE);
> +
> + /* 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 */
> + virCommandAddArg(cmd, "-I"); /* Present ioapic to the guest */
> + virCommandAddArg(cmd, "-H"); /* vmexit from guest on hlt */
> + virCommandAddArg(cmd, "-P"); /* vmexit from guest on pause */
> +
> + /* Devices */
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArg(cmd, "0:0,hostbridge");
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArgFormat(cmd, "1:0,virtio-net,%s", realifname);
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArgFormat(cmd, "2:0,ahci-hd,%s", disk->src);
> + virCommandAddArg(cmd, "-S");
> + virCommandAddArg(cmd, "31,uart");
> + virCommandAddArg(cmd, vm->def->name);
> +
> + return cmd;
> +}
On the basis that experiance has shown building command line args get very
long & complex very quickly, I'd suggest you have a separate byhve_command.{ch}
for the helper APIs doing command line building/parsing. Also worth splitting
up the method into separate methods for adding disks, NICs, and other types of
devices.
When building a command line you want to use 'virReportError(VIR_ERR_CONFIG_UNSUPPORTED'
as much as possible if you find something in the XML that you can't support right now.
That ensures that if the user gives you unexpected XML they'll get an error instead of
having part of their XML ignored.
> +
> +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 you put that check in the method for building the command line for
disks so everything's in one place.
> +
> + if (virAsprintf(&logfile, "%s/%s.log",
> + BHYVE_LOG_DIR, vm->def->name) < 0)
> + return -1;
> +
> +
> + if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT,
> + S_IRUSR|S_IWUSR)) < 0) {
> + virReportSystemError(errno,
> + _("Failed to open '%s'"),
> + logfile);
> + goto cleanup;
> + }
> +
> + /* Call bhyveload to load a VM */
> + if (!(cmd = virBhyveProcessBuildLoadCmd(driver,
> + vm)))
> + goto cleanup;
> + virCommandSetOutputFD(cmd, &logfd);
> + virCommandSetErrorFD(cmd, &logfd);
> +
> + /* Log generated command line */
> + virCommandWriteArgLog(cmd, logfd);
> + if ((pos = lseek(logfd, 0, SEEK_END)) < 0)
> + VIR_WARN("Unable to seek to end of logfile: %s",
> + virStrerror(errno, ebuf, sizeof(ebuf)));
> +
> + VIR_INFO("Loading domain '%s'", vm->def->name);
> + if (virCommandRun(cmd, &status) < 0)
> + goto cleanup;
> +
> + if (status != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Guest failed to load: %d"), status);
> + goto cleanup;
> + }
> +
> + virCommandFree(cmd);
> +
> + VIR_FREE(privconn->pidfile);
> + if (!(privconn->pidfile = virPidFileBuildPath(BHYVE_STATE_DIR,
> + vm->def->name))) {
> + virReportSystemError(errno,
> + "%s", _("Failed to build pidfile path."));
> + goto cleanup;
> + }
> +
> + if (unlink(privconn->pidfile) < 0 &&
> + errno != ENOENT) {
> + virReportSystemError(errno,
> + _("Cannot remove state PID file %s"),
> + privconn->pidfile);
> + goto cleanup;
> + }
> +
> + /* Call bhyve to start the VM */
> + if (!(cmd = virBhyveProcessBuildBhyveCmd(driver,
> + vm)))
> + goto cleanup;
> + virCommandSetOutputFD(cmd, &logfd);
> + virCommandSetErrorFD(cmd, &logfd);
> + virCommandWriteArgLog(cmd, logfd);
> + virCommandSetPidFile(cmd, privconn->pidfile);
> + virCommandDaemonize(cmd);
> +
> + VIR_INFO("Starting domain '%s'", vm->def->name);
> + ret = virCommandRun(cmd, NULL);
Interesting, so you've got a 2 step process for starting VMs. What
happens if the second step fails. Do you need to undo the 1st step
in someway ?
> +
> + if (ret == 0) {
> + if (virPidFileReadPath(privconn->pidfile, &vm->pid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Domain %s didn't show up"), vm->def->name);
> + goto cleanup;
> + }
> + } else {
> + goto cleanup;
> + }
> +
> +cleanup:
> + VIR_FREE(logfile);
> + return ret;
> +}
> +
> +int
> +virBhyveProcessStop(bhyveConnPtr driver,
> + virDomainObjPtr vm,
> + virDomainShutoffReason reason ATTRIBUTE_UNUSED)
> +{
> + size_t i;
> + int ret = -1;
> + int status;
> + virCommandPtr cmd = NULL;
> +
You should add a virDomainObjIsActive() check here to protect yourself
against being called when the VM is already stopped which could otherwise
lead to kill(-1) which is a bad thing :-)
> + /* 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;
> +}
Overall it looks like you're heading in the right direction here.
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