Re: [PATCH 1/2] bhyve: Support /domain/bootloader configuration for non-FreeBSD guests.

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

 



On 23.10.2014 21:58, Conrad Meyer wrote:
Also, flip Bhyve /domain/os/type support from HVM to Xen. Bhyve only
supports paravirtualized guests, and 'xen' is closest to that. We still
default to bhyveloader(1) if no explicit bootloader configuration is
supplied in the domain.

If the /domain/bootloader looks like grub-bhyve and the user doesn't
supply /domain/bootloader_args, we make an intelligent guess and try
chainloading the first partition on the disk.

Caveats:
   - We can't install from CD without explicit bootloader_args.
   - We leave a device.map file lying around in /tmp. I don't see a good
     way not to do so without reworking the API somewhat.

Sponsored by:   EMC / Isilon storage division

Signed-off-by: Conrad Meyer <conrad.meyer@xxxxxxxxxx>
---
  docs/drvbhyve.html.in                              |  30 +++++-
  docs/formatdomain.html.in                          |   4 +-
  po/libvirt.pot                                     |   4 +
  src/bhyve/bhyve_capabilities.c                     |   2 +-
  src/bhyve/bhyve_command.c                          | 107 +++++++++++++++++++--
  tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.xml |   2 +-
  tests/bhyvexml2argvdata/bhyvexml2argv-base.xml     |   2 +-
  tests/bhyvexml2argvdata/bhyvexml2argv-console.xml  |   2 +-
  .../bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.xml |   2 +-
  .../bhyvexml2argv-disk-virtio.xml                  |   2 +-
  tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.xml  |   2 +-
  tests/bhyvexml2argvdata/bhyvexml2argv-metadata.xml |   2 +-
  tests/bhyvexml2argvdata/bhyvexml2argv-serial.xml   |   2 +-
  .../bhyvexml2xmlout-metadata.xml                   |   2 +-
  14 files changed, 139 insertions(+), 26 deletions(-)

diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in
index 39afdf5..c6c79d7 100644
--- a/docs/drvbhyve.html.in
+++ b/docs/drvbhyve.html.in
@@ -37,8 +37,7 @@ bhyve+ssh://root@xxxxxxxxxxx/system (remote access, SSH tunnelled)
  <h3>Example config</h3>
  <p>
  The bhyve driver in libvirt is in its early stage and under active development. So it supports
-only limited number of features bhyve provides. All the supported features could be found
-in this sample domain XML.
+only limited number of features bhyve provides.
  </p>

  <p>
@@ -50,13 +49,13 @@ up to 31 PCI devices.

  <pre>
  &lt;domain type='bhyve'&gt;
-  &lt;name&gt;bhyve&lt;/name&gt;
-  &lt;uuid&gt;df3be7e7-a104-11e3-aeb0-50e5492bd3dc&lt;/uuid&gt;
+    &lt;name&gt;bhyve&lt;/name&gt;
+    &lt;uuid&gt;df3be7e7-a104-11e3-aeb0-50e5492bd3dc&lt;/uuid&gt;
      &lt;memory&gt;219136&lt;/memory&gt;
      &lt;currentMemory&gt;219136&lt;/currentMemory&gt;
      &lt;vcpu&gt;1&lt;/vcpu&gt;
      &lt;os&gt;
-       &lt;type&gt;hvm&lt;/type&gt;
+       &lt;type&gt;xen&lt;/type&gt;
      &lt;/os&gt;
      &lt;features&gt;
        &lt;apic/&gt;
@@ -157,5 +156,26 @@ An example of domain XML device entry for that will look like:</p>
  <p>Please refer to the <a href="storage.html">Storage documentation</a> for more details on storage
  management.</p>

+<h3><a name="grubbhyve">Using grub2-bhyve or Alternative Bootloaders</a></h3>
+
+<p>It's possible to boot non-FreeBSD guests by specifying an explicit
+bootloader, e.g. <code>grub-bhyve(1)</code>. Arguments to the bootloader may be
+specified as well. If no arguments are given and bootloader is
+<code>grub-bhyve</code>, libvirt will try and boot from the first partition of
+the disk image.</p>
+
+<pre>
+  ...
+    &lt;bootloader&gt;/usr/local/sbin/grub-bhyve&lt;/bootloader&gt;
+    &lt;bootloader_args&gt;...&lt;/bootloader_args&gt;
+  ...
+</pre>
+
+<p>(Of course, to install from a CD a user will have to supply explicit
+arguments to <code>grub-bhyve</code>.)</p>
+
+<p>Caveat: <code>bootloader_args</code> does not support any quoting.
+Filenames, etc, must not have spaces or they will be tokenized incorrectly.</p>
+
    </body>
  </html>
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0099ce7..b7b6c46 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -217,7 +217,9 @@
        a BIOS, and instead the host is responsible to kicking off the
        operating system boot. This may use a pseudo-bootloader in the
        host to provide an interface to choose a kernel for the guest.
-      An example is <code>pygrub</code> with Xen.
+      An example is <code>pygrub</code> with Xen. The Bhyve hypervisor
+      also uses a host bootloader, either <code>bhyveload</code> or
+      <code>grub-bhyve</code>.
      </p>

  <pre>
diff --git a/po/libvirt.pot b/po/libvirt.pot
index 0b44ad7..d8c9a4d 100644
--- a/po/libvirt.pot
+++ b/po/libvirt.pot
@@ -851,6 +851,10 @@ msgstr ""
  msgid "domain should have at least one disk defined"
  msgstr ""

+#: src/bhyve/bhyve_command.c:407
+msgid "Custom loader requires explicit %s configuration"
+msgstr ""
+
  #: src/bhyve/bhyve_device.c:50
  msgid "PCI bus 0 slot 1 is reserved for the implicit LPC PCI-ISA bridge"
  msgstr ""

There's no need for this. The po/* files are regenerated on the release.

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 132ce91..b37a24f 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -85,7 +85,7 @@ virBhyveCapsBuild(void)
                                     false, false)) == NULL)
          return NULL;

-    if ((guest = virCapabilitiesAddGuest(caps, "hvm",
+    if ((guest = virCapabilitiesAddGuest(caps, "xen",
                                           VIR_ARCH_X86_64,
                                           "bhyve",
                                           NULL, 0, NULL)) == NULL)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
index bea4a59..99956ae 100644
--- a/src/bhyve/bhyve_command.c
+++ b/src/bhyve/bhyve_command.c
@@ -300,6 +300,7 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn,
  {
      virCommandPtr cmd;
      virDomainDiskDefPtr disk;
+    bool bhyveload, grub_bhyve;

      if (def->ndisks < 1) {
          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -326,19 +327,105 @@ virBhyveProcessBuildLoadCmd(virConnectPtr conn,
          return NULL;
      }

-    cmd = virCommandNew(BHYVELOAD);
+    if (def->os.bootloader == NULL) {
+        bhyveload = true;
+        grub_bhyve = false;
+        cmd = virCommandNew(BHYVELOAD);
+    } else {
+        bhyveload = false;
+        if (strstr(def->os.bootloader, "grub-bhyve") == 0)
+            grub_bhyve = true;
+        cmd = virCommandNew(def->os.bootloader);
+    }

-    /* Memory */
-    virCommandAddArg(cmd, "-m");
-    virCommandAddArgFormat(cmd, "%llu",
-                           VIR_DIV_UP(def->mem.max_balloon, 1024));
+    if (bhyveload && def->os.bootloaderArgs == NULL) {
+        VIR_DEBUG("%s: bhyveload with default arguments", __func__);

VIR_DEBUG() itself will report what function has it been called from.

+
+        /* Memory (MB) */
+        virCommandAddArg(cmd, "-m");
+        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);
+    } else if (grub_bhyve && def->os.bootloaderArgs == NULL) {
+        char tmpmapfile[128] = "/tmp/grub-bhyve-device.map_XXXXXX";

This doesn't look nice. I'd prefer char *tmpmapfile combined with virAsprintf() and VIR_FREE().

+        FILE *f;
+        int fd;
+
+        VIR_DEBUG("%s: grub-bhyve with default arguments", __func__);
+
+        /*
+         * XXX Default grub-bhyve has some BIG caveats, but MAY work for some
+         * typical configurations. In particular:
+         *
+         *   - Can't create a new VM this way (no CD, no boot from CD)
+         *   - Assumes a GRUB install on hd0,msdos1
+         */

-    /* Image path */
-    virCommandAddArg(cmd, "-d");
-    virCommandAddArg(cmd, virDomainDiskGetSource(disk));
+        /* XXX cleanup this file. */
+        fd = mkstemp(tmpmapfile);

No, we use mkostemp( .. , O_CLOEXEC) instead.

+        if (fd < 0) {
+                virReportError(VIR_ERR_OPEN_FAILED, tmpmapfile);
+                goto error;
+        }
+
+        f = VIR_FDOPEN(fd, "wb+");
+        if (f == NULL) {
+                VIR_FORCE_CLOSE(fd);
+                virReportError(VIR_ERR_OPEN_FAILED, tmpmapfile);
+                goto error;
+        }
+
+        /* Grub device.map */
+        fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk));
+        /* XXX CDs would look like: "(cd0) /path/to/CD" */
+
+        if (VIR_FCLOSE(f) < 0) {
+                virReportSystemError(errno, "%s", _("failed to close file"));
+                goto error;
+        }

-    /* VM name */
-    virCommandAddArg(cmd, def->name);
+
+        virCommandAddArg(cmd, "--device-map");
+        virCommandAddArg(cmd, tmpmapfile);
+
+        /* Memory in MB */
+        virCommandAddArg(cmd, "--memory");
+        virCommandAddArgFormat(cmd, "%llu",
+                               VIR_DIV_UP(def->mem.max_balloon, 1024));
+
+        /* To boot from CD, "cd0" here. */
+        virCommandAddArg(cmd, "--root");
+        virCommandAddArg(cmd, "hd0,msdos1");
+
+        /* VM name */
+        virCommandAddArg(cmd, def->name);
+    } else if (def->os.bootloaderArgs == NULL) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Custom loader requires explicit %s configuration"),
+                       "bootloader_args");
+        goto error;
+    } else {
+        char **blargs, **arg;
+
+        VIR_DEBUG("%s: custom loader '%s' with arguments", __func__,
+                  def->os.bootloader);
+
+        /* XXX: Handle quoted? */
+        blargs = virStringSplit(def->os.bootloaderArgs, " ", 0);
+        for (arg = blargs; *arg; arg++)
+                virCommandAddArg(cmd, *arg);
+        virStringFreeList(blargs);

I don't think it's safe to pass arbitrary arguments from XML. I find this too critical to ACK the patch, buy maybe further discussion can change my mind.

+    }

      return cmd;
+
+error:
+    virCommandFree(cmd);
+    return NULL;

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]