Re: [libvirt] [PATCHv8 1/7] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.

11 Nov 2014


      On Tue, Nov 11, 2014 at 9:52 AM, Michal Privoznik <mprivozn@redhat.com> wrote:
...
On 08.11.2014 17:48, Conrad Meyer wrote:
...
+static virCommandPtr
+virBhyveProcessBuildBhyveloadCmd(virDomainDefPtr def, virDomainDiskDefPtr
disk)
  {
      virCommandPtr cmd;
-    virDomainDiskDefPtr disk;
-    if (def->ndisks < 1) {
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("domain should have at least one disk
defined"));
+    cmd = virCommandNew(BHYVELOAD);
+
+    if (def->os.bootloaderArgs == NULL) {
+        VIR_DEBUG("bhyveload with default arguments");
+
+        /* Memory (MB) */
+        virCommandAddArg(cmd, "-m");
+        virCommandAddArgFormat(cmd, "%llu",
+                               VIR_DIV_UP(def->mem.max_balloon, 1024));
One of the things I'm worried about is, if the boot args are not specified
we properly add memory configured in domain XML.
...
+
+        /* Image path */
+        virCommandAddArg(cmd, "-d");
+        virCommandAddArg(cmd, virDomainDiskGetSource(disk));
+
+        /* VM name */
+        virCommandAddArg(cmd, def->name);
+    } else {
+        VIR_DEBUG("bhyveload with arguments");
+        virAppendBootloaderArgs(cmd, def);
+    }
+
However, IIUC the memory amount can be overridden with boot args. Is that
expected?
Yes. If you use bootloader_args, you get exactly what you ask for and no more.
...
...
+static virCommandPtr
+virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def,
+                                 virConnectPtr conn,
+                                 const char *devmap_file,
+                                 char **devicesmap_out)
+{
+    virDomainDiskDefPtr disk, cd;
+    virBuffer devicemap;
+    virCommandPtr cmd;
+    size_t i;
+
+    if (def->os.bootloaderArgs != NULL)
+        return virBhyveProcessBuildCustomLoaderCmd(def);
+
+    devicemap = (virBuffer)VIR_BUFFER_INITIALIZER;
+
+    /* Search disk list for CD or HDD device. */
+    cd = disk = NULL;
+    for (i = 0; i < def->ndisks; i++) {
+        if (!virBhyveUsableDisk(conn, def->disks[i]))
+            continue;
+
+        if (cd == NULL &&
+            def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
+            cd = def->disks[i];
+            VIR_INFO("Picking %s as boot CD",
virDomainDiskGetSource(cd));
+        }
+
+        if (disk == NULL &&
+            def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
+            disk = def->disks[i];
+            VIR_INFO("Picking %s as HDD", virDomainDiskGetSource(disk));
+        }
Can we utilize <boot order='X'/> attribute here?
This has come up before and the answer is yes, but I'd prefer to do so
in a later patch series; this one is already growing unwieldy.
...
...
+    }
+
+    cmd = virCommandNew(def->os.bootloader);
+
+    VIR_DEBUG("grub-bhyve with default arguments");
+
+    if (devicesmap_out != NULL) {
+        /* Grub device.map (just for boot) */
+        if (disk != NULL)
+            virBufferAsprintf(&devicemap, "(hd0) %s\n",
+                              virDomainDiskGetSource(disk));
+
+        if (cd != NULL)
+            virBufferAsprintf(&devicemap, "(cd) %s\n",
+                              virDomainDiskGetSource(cd));
+
+        *devicesmap_out = virBufferContentAndReset(&devicemap);
+    }
+
+    if (cd != NULL) {
+        virCommandAddArg(cmd, "--root");
+        virCommandAddArg(cmd, "cd");
+    } else {
+        virCommandAddArg(cmd, "--root");
+        virCommandAddArg(cmd, "hd0,msdos1");
+    }
+
+    virCommandAddArg(cmd, "--device-map");
+    virCommandAddArg(cmd, devmap_file);
+
+    /* Memory in MB */
+    virCommandAddArg(cmd, "--memory");
      virCommandAddArgFormat(cmd, "%llu",
                             VIR_DIV_UP(def->mem.max_balloon, 1024));
-    /* Image path */
-    virCommandAddArg(cmd, "-d");
-    virCommandAddArg(cmd, virDomainDiskGetSource(disk));
-
      /* VM name */
      virCommandAddArg(cmd, def->name);
return cmd;
  }
Otherwise looking good.
Michal
Thanks!

Conrad