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

Re: [libvirt] [PATCH v7 1/2] bhyve: add a basic driver



On Sun, Feb 09, 2014 at 06:46:12PM +0400, Roman Bogorodskiy wrote:

> +static int
> +bhyveBuildDiskArgStr(const virDomainDef *def, virCommandPtr cmd)
> +{
> +    virDomainDiskDefPtr disk;
> +
> +    if (def->ndisks != 1) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("domain should have one and only one disk defined"));
> +        return -1;
> +    }
> +
> +    disk = def->disks[0];
> +
> +    virCommandAddArg(cmd, "-s");
> +    virCommandAddArgFormat(cmd, "2:0,ahci-hd,%s", disk->src);

Before accessing disk->src, you need to check what disk->type
is - the 'src' field is only valid for file/block backed disks,
not network backed ones.

Also based on the fact that your arg contains the string 'ahci'
is is correct to say this is exposing a SATA bus disk ?

If so, you also want to validate disk->bus == VIR_DOMAIN_DISK_BUS_SATA
and disk->device == VIR_DOMAIN_DISK_DEVICE_DISK and report
CONFIG_UNSUPPORTED in both cases.


> +    /* Clarification about -H and -P flags from Peter Grehan:
> +     * -H and -P flags force the guest to exit when it executes IA32 HLT and PAUSE
> +     * instructions respectively.
> +     *
> +     * For the HLT exit, bhyve uses that to infer that the guest is idling and can
> +     * be put to sleep until an external event arrives. If this option is not used,
> +     * the guest will always use 100% of CPU on the host.
> +     *
> +     * The PAUSE exit is most useful when there are large numbers of guest VMs running,
> +     * since it forces the guest to exit when it spins on a lock acquisition.
> +     */
> +    virCommandAddArg(cmd, "-H"); /* vmexit from guest on hlt */
> +    virCommandAddArg(cmd, "-P"); /* vmexit from guest on pause */

Ok, agree that both of those make sense to enable unconditionally.

> +    virCommandAddArg(cmd, "-S");
> +    virCommandAddArg(cmd, "31,uart");

Hmm, this is enabling a serial port right ? If so, you only want
todo this if you have def->nserials == 1 I reckon. What is the
string '31' saying ?

> +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);

Curious, so you have to repeat the disk specification in two commands.

Same comment about validating the disk->type before accessing 'src'


> diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c
> new file mode 100644
> index 0000000..e8e082b
> --- /dev/null
> +++ b/src/bhyve/bhyve_driver.c
> +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;

This means that you'll never be able to default to "bhyve"
when URI == NULL. Generally we have this auto-detected
eg


   if (conn->uri == NULL) {
        if (bhyve_driver == NULL) {
            return VIR_DRV_OPEN_DECLINED
        }

        ...accept...
   }  else {
       ...all the logic below....

See qemuConnectOpen for an example of logic to follow

> +
> +     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;
> +     }
> +
> +     if (bhyve_driver == NULL) {
> +         virReportError(VIR_ERR_INTERNAL_ERROR,
> +                        "%s", _("bhyve state driver is not active"));
> +         return VIR_DRV_OPEN_ERROR;
> +     }
> +
> +     conn->privateData = bhyve_driver;
> +
> +     return VIR_DRV_OPEN_SUCCESS;
> +}


> +static virDomainPtr
> +bhyveDomainDefineXML(virConnectPtr conn, const char *xml)
> +{
> +    bhyveConnPtr privconn = conn->privateData;
> +    virDomainPtr dom = NULL;
> +    virDomainDefPtr def = NULL;
> +    virDomainDefPtr oldDef = NULL;
> +    virDomainObjPtr vm = NULL;
> +
> +    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;
> +    }

Nit-pick: we avoid {} for a single-line body

> +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;
> +}



> +int
> +virBhyveProcessStart(virConnectPtr conn,
> +                     bhyveConnPtr driver,
> +                     virDomainObjPtr vm,
> +                     virDomainRunningReason reason)

> +    VIR_INFO("Loading domain '%s'", vm->def->name);
> +    if (virCommandRun(cmd, &status) < 0)
> +        goto cleanup;

Now the domain is loaded...

> +
> +    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;

I'd suggest this building is done before loading the domain
so you don't needlessly load the domain before we detect any
possible user configuration errors.


Regards,
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]