On Thu, Feb 13, 2014 at 02:14:32PM +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];
> +
> + if (disk->bus != VIR_DOMAIN_DISK_BUS_SATA) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unsupported disk bus type"));
> + return -1;
> + }
> +
> + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unsupported disk type"));
> + return -1;
> + }
We still need to validate disk->type before accessing disk->src
otherwise you'll crash if someone uses type=network
> +
> + virCommandAddArg(cmd, "-s");
> + virCommandAddArgFormat(cmd, "2:0,ahci-hd,%s", disk->src);
What is the '2:0' bit here ? Is that disk controller/unit
number ?
> +virCommandPtr
> +virBhyveProcessBuildLoadCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED,
> + virDomainObjPtr vm)
> +{
> + virCommandPtr cmd;
> + virDomainDiskDefPtr disk;
> +
> + if (vm->def->ndisks != 1) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain should have one and only one disk defined"));
> + return NULL;
> + }
> +
> + disk = vm->def->disks[0];
> +
> + if (disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("unsupported disk type"));
> + return NULL;
> + }
And validate disk->type again here
> +
> + 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 char *
> +bhyveConnectGetCapabilities(virConnectPtr conn)
> +{
> + bhyveConnPtr privconn = conn->privateData;
> + char *xml;
> +
> + if (virConnectGetCapabilitiesEnsureACL(conn) < 0)
> + return NULL;
> +
> + bhyveDriverLock(privconn);
> + if ((xml = virCapabilitiesFormatXML(privconn->caps)) == NULL)
> + virReportOOMError();
> + bhyveDriverUnlock(privconn);
> +
> + return xml;
> +}
Technically the lock/unlock isn't needed since you never
change privconn->caps once you've created it.
> +static virDrvOpenStatus
> +bhyveConnectOpen(virConnectPtr conn,
> + virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> + unsigned int flags)
> +{
> + virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
> +
> + if (conn->uri == NULL) {
> + if (bhyve_driver == NULL)
> + return VIR_DRV_OPEN_DECLINED;
> +
> + if (!(conn->uri = virURIParse("bhyve:///system")))
> + return VIR_DRV_OPEN_ERROR;
Nitpick indentation is too great in the two 'return' lines
> + } else {
> + 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;
> + }
> + }
> +
> + if (virConnectOpenEnsureACL(conn) < 0)
> + 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"));
Remove the 'virReportError' call, since that's already done for you
by the parsing code.
> + goto cleanup;
> + }
> +
> + if (virDomainDefineXMLEnsureACL(conn, def) < 0)
> + goto cleanup;
> +
> + if (!(vm = virDomainObjListAdd(privconn->domains, def,
> + privconn->xmlopt,
> + 0, &oldDef)))
> + goto cleanup;
> + def = NULL;
> +
> + 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);
> +
> + return dom;
> +}
> +
> +
> +int
> +virBhyveProcessStart(virConnectPtr conn,
> + bhyveConnPtr driver,
> + virDomainObjPtr vm,
> + virDomainRunningReason reason)
> +{
> + char *logfile = NULL;
> + int logfd = -1;
> + off_t pos = -1;
> + char ebuf[1024];
> + virCommandPtr cmd = NULL;
> + virCommandPtr load_cmd = NULL;
> + bhyveConnPtr privconn = conn->privateData;
> + int ret = -1, status;
> +
> + 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;
> + }
> +
> + 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);
> +
> + /* Now bhyve command is constructed, meaning the
> + * domain is ready to be started, so we can build
> + * and execute bhyveload command */
> + if (!(load_cmd = virBhyveProcessBuildLoadCmd(driver,
> + vm)))
> + goto cleanup;
> + virCommandSetOutputFD(load_cmd, &logfd);
> + virCommandSetErrorFD(load_cmd, &logfd);
> +
> + /* Log generated command line */
> + virCommandWriteArgLog(load_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(load_cmd, &status) < 0)
> + goto cleanup;
> +
> + if (status != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Guest failed to load: %d"), status);
> + goto cleanup;
> + }
> +
> + /* Now we can start the domain */
> + VIR_INFO("Starting domain '%s'", vm->def->name);
I'd suggest s/INFO/DEBUG/ here and earlier. If you want user visible
log messages about key operations, then we should talk about what is
needed to make viraudit.{c,h} work on FreeBSD, and use domain_audit.c
for this.
> + ret = virCommandRun(cmd, NULL);
> +
> + 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;
> + }
> +
> + vm->def->id = vm->pid;
> + virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, reason);
> + } else {
> + goto cleanup;
> + }
> +
> +cleanup:
> + if (ret < 0) {
> + virCommandPtr destroy_cmd;
> + if ((destroy_cmd = virBhyveProcessBuildDestroyCmd(driver, vm)) != NULL) {
> + virCommandSetOutputFD(load_cmd, &logfd);
> + virCommandSetErrorFD(load_cmd, &logfd);
> + ignore_value(virCommandRun(destroy_cmd, NULL));
> + virCommandFree(destroy_cmd);
> + }
> + }
> +
> + virCommandFree(load_cmd);
> + virCommandFree(cmd);
> + VIR_FREE(logfile);
> + if (VIR_CLOSE(logfd) < 0) {
> + virReportSystemError(errno, "%s", _("could not close logfile"));
> + }
You can use VIR_FORCE_CLOSE and ignore the error message here.
> + return ret;
> +}
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list