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