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

Re: [libvirt] [PATCH v3] bhyve: add a basic driver



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


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