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

17 Feb 2014


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

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

Daniel P. Berrange