[libvirt] [PATCHv5 0/6] Add non-FreeBSD guest support to Bhyve driver.
Drvbhyve hardcodes bhyveload(8) as the host bootloader for guests. bhyveload(8) loader only supports FreeBSD guests. This patch series adds <bootloader> and <bootloader_args> handling to bhyve_command, so libvirt can boot non-FreeBSD guests in Bhyve. Additionally, support for grub-bhyve(1)'s --cons-dev argument is added so that interactive GRUB menus can be manipulated with the domain-configured serial device. See patch logs for further details. Thanks, Conrad Changelog for v5: - Added loader arguments tests to the bhyvexml2argvtest harness - Dropped po/pot file changes per Daniel - Added bootloader, bootloader_args to domain XML schema - Added xml2argv tests for new bhyve bootloader handling - Added xml2argv test for grub with console device Conrad Meyer (6): bhyve: Support /domain/bootloader configuration for non-FreeBSD guests. bhyvexml2argv: Add loader argv tests. domaincommon.rng: Add bootloader, bootloader_arg to os=hvm schema for Bhyve bhyvexml2argv: Add tests for domain-configured bootloader, args bhyve: Add console support for grub-bhyve bootloader bhyvexml2argv: Add test for grub console support docs/drvbhyve.html.in | 94 ++++++++- docs/formatdomain.html.in | 4 +- docs/schemas/domaincommon.rng | 41 ++-- src/bhyve/bhyve_command.c | 215 +++++++++++++++++++-- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_domain.c | 5 + src/bhyve/bhyve_domain.h | 1 + src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 13 +- .../bhyvexml2argv-acpiapic.ldargs | 1 + tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs | 1 + .../bhyvexml2argv-bhyveload-explicitargs.args | 3 + .../bhyvexml2argv-bhyveload-explicitargs.ldargs | 1 + .../bhyvexml2argv-bhyveload-explicitargs.xml | 23 +++ .../bhyvexml2argvdata/bhyvexml2argv-console.ldargs | 1 + .../bhyvexml2argv-custom-loader.args | 3 + .../bhyvexml2argv-custom-loader.ldargs | 1 + .../bhyvexml2argv-custom-loader.xml | 24 +++ .../bhyvexml2argv-disk-cdrom-grub.args | 3 + .../bhyvexml2argv-disk-cdrom-grub.ldargs | 2 + .../bhyvexml2argv-disk-cdrom-grub.xml | 23 +++ .../bhyvexml2argv-disk-cdrom.ldargs | 1 + .../bhyvexml2argv-disk-virtio.ldargs | 1 + .../bhyvexml2argv-grub-defaults.args | 3 + .../bhyvexml2argv-grub-defaults.ldargs | 2 + .../bhyvexml2argv-grub-defaults.xml | 23 +++ .../bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs | 1 + .../bhyvexml2argv-serial-grub.args | 4 + .../bhyvexml2argv-serial-grub.ldargs | 2 + .../bhyvexml2argv-serial-grub.xml | 26 +++ .../bhyvexml2argvdata/bhyvexml2argv-serial.ldargs | 1 + tests/bhyvexml2argvtest.c | 41 +++- 32 files changed, 527 insertions(+), 44 deletions(-) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs -- 1.9.3
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 (or a CD if one exists, under the assumption that for a VM a CD is likely an install source). Caveat: Assumes the HDD boots from the msdos1 partition. I think this is a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload already assumes that the first disk should be booted.) I've tested both HDD and CD boot and they seem to work. Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/drvbhyve.html.in | 94 ++++++++++++++++++++- docs/formatdomain.html.in | 4 +- src/bhyve/bhyve_command.c | 202 +++++++++++++++++++++++++++++++++++++++++----- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_domain.c | 5 ++ src/bhyve/bhyve_domain.h | 1 + src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 13 ++- 8 files changed, 298 insertions(+), 28 deletions(-) diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in index 39afdf5..ee1317e 100644 --- a/docs/drvbhyve.html.in +++ b/docs/drvbhyve.html.in @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/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> @@ -48,10 +47,21 @@ disk device were supported per-domain. However, up to 31 PCI devices. </p> +<p> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you +want to install from CD, put the CD device first. If not, put the root HDD +first. +</p> + +<p> +Note: Only the SATA bus is supported. Only <code>cdrom</code>- and +<code>disk</code>-type disks are supported. +</p> + <pre> <domain type='bhyve'> - <name>bhyve</name> - <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> <memory>219136</memory> <currentMemory>219136</currentMemory> <vcpu>1</vcpu> @@ -76,6 +86,7 @@ up to 31 PCI devices. <driver name='file' type='raw'/> <source file='/path/to/cdrom.iso'/> <target dev='hdc' bus='sata'/> + <readonly/> </disk> <interface type='bridge'> <model type='virtio'/> @@ -85,6 +96,53 @@ up to 31 PCI devices. </domain> </pre> +<p>(The <disk> sections may be swapped in order to install from +<em>cdrom.iso</em>.)</p> + +<h3>Example config (Linux guest)</h3> + +<p> +Note the addition of <bootloader>. +</p> + +<pre> +<domain type='bhyve'> + <name>linux_guest</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>131072</memory> + <currentMemory>131072</currentMemory> + <vcpu>1</vcpu> + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <os> + <type>hvm</type> + </os> + <features> + <apic/> + <acpi/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <disk type='file' device='disk'> + <driver name='file' type='raw'/> + <source file='/path/to/guest_hdd.img'/> + <target dev='hda' bus='sata'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file' type='raw'/> + <source file='/path/to/cdrom.iso'/> + <target dev='hdc' bus='sata'/> + <readonly/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + </interface> + </devices> +</domain> +</pre> <h2><a name="usage">Guest usage / management</a></h2> @@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p> <pre>start --console domname</pre> +<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p> + +<pre>cu -l /dev/nmdm0B</pre> + <h3><a name="xmltonative">Converting from domain XML to Bhyve args</a></h3> <p> @@ -157,5 +223,25 @@ 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 the bootloader is <code>grub-bhyve</code> and arguments +are omitted, libvirt will try and boot the first disk in the domain (either +<code>cdrom</code>- or <code>disk</code>-type devices). If the disk type is +<code>disk</code>, it will attempt to boot from the first partition in the disk +image.</p> + +<pre> + ... + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <bootloader_args>...</bootloader_args> + ... +</pre> + +<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/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index bea4a59..01f1795 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -26,6 +26,8 @@ #include <net/if_tap.h> #include "bhyve_command.h" +#include "bhyve_domain.h" +#include "datatypes.h" #include "viralloc.h" #include "virfile.h" #include "virstring.h" @@ -294,51 +296,215 @@ virBhyveProcessBuildDestroyCmd(bhyveConnPtr driver ATTRIBUTE_UNUSED, return cmd; } -virCommandPtr -virBhyveProcessBuildLoadCmd(virConnectPtr conn, - virDomainDefPtr def) +static void +virAppendBootloaderArgs(virCommandPtr cmd, virDomainDefPtr def) +{ + char **blargs; + + /* XXX: Handle quoted? */ + blargs = virStringSplit(def->os.bootloaderArgs, " ", 0); + virCommandAddArgSet(cmd, (const char * const *)blargs); + virStringFreeList(blargs); +} + +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)); + + /* Image path */ + virCommandAddArg(cmd, "-d"); + virCommandAddArg(cmd, virDomainDiskGetSource(disk)); + + /* VM name */ + virCommandAddArg(cmd, def->name); + } else { + VIR_DEBUG("bhyveload with arguments"); + virAppendBootloaderArgs(cmd, def); + } + + return cmd; +} + +static virCommandPtr +virBhyveProcessBuildCustomLoaderCmd(virDomainDefPtr def) +{ + virCommandPtr cmd; + + if (def->os.bootloaderArgs == NULL) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Custom loader requires explicit %s configuration"), + "bootloader_args"); return NULL; } - disk = def->disks[0]; + VIR_DEBUG("custom loader '%s' with arguments", def->os.bootloader); + + cmd = virCommandNew(def->os.bootloader); + virAppendBootloaderArgs(cmd, def); + return cmd; +} + +static bool +virBhyveUsableDisk(virConnectPtr conn, virDomainDiskDefPtr disk) +{ if (virStorageTranslateDiskSourcePool(conn, disk) < 0) - return NULL; + return false; if ((disk->device != VIR_DOMAIN_DISK_DEVICE_DISK) && (disk->device != VIR_DOMAIN_DISK_DEVICE_CDROM)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported disk device")); - return NULL; + return false; } if ((virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_FILE) && (virDomainDiskGetType(disk) != VIR_STORAGE_TYPE_VOLUME)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported disk type")); - return NULL; + return false; } - cmd = virCommandNew(BHYVELOAD); + return true; +} - /* Memory */ - virCommandAddArg(cmd, "-m"); +static virCommandPtr +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv, + virConnectPtr conn) +{ + virDomainDiskDefPtr disk, cd; + virCommandPtr cmd; + size_t i; + FILE *f; + int rc; + + if (def->os.bootloaderArgs != NULL) + return virBhyveProcessBuildCustomLoaderCmd(def); + + /* 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)); + } + } + + cmd = virCommandNew(def->os.bootloader); + + VIR_DEBUG("grub-bhyve with default arguments"); + + if (priv != NULL) { + rc = virAsprintf(&priv->grub_devicesmap_file, + "%s/grub_bhyve-%s-device.map", BHYVE_STATE_DIR, + def->name); + if (rc < 0) + goto error; + + f = fopen(priv->grub_devicesmap_file, "wb"); + if (f == NULL) { + virReportSystemError(errno, _("Failed to open '%s'"), + priv->grub_devicesmap_file); + goto error; + } + + /* Grub device.map (just for boot) */ + if (disk != NULL) + fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk)); + + if (cd != NULL) + fprintf(f, "(cd) %s\n", virDomainDiskGetSource(cd)); + + if (VIR_FCLOSE(f) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + goto error; + } + } + + if (cd != NULL) { + VIR_WARN("Trying to boot cd with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "cd"); + } else { + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "hd0,msdos1"); + } + + virCommandAddArg(cmd, "--device-map"); + if (priv != NULL) + virCommandAddArg(cmd, priv->grub_devicesmap_file); + else + virCommandAddArg(cmd, "<device.map>"); + + /* 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; + + error: + virCommandFree(cmd); + return NULL; +} + +virCommandPtr +virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv) +{ + virDomainDiskDefPtr disk; + + if (def->ndisks < 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("domain should have at least one disk defined")); + return NULL; + } + + if (def->os.bootloader == NULL) { + disk = def->disks[0]; + + if (!virBhyveUsableDisk(conn, disk)) + return NULL; + + if (def->ndisks > 1) { + VIR_WARN("drvbhyve attempting to boot from device %s of multiple-" + "device configuration", virDomainDiskGetSource(disk)); + } + + return virBhyveProcessBuildBhyveloadCmd(def, disk); + } else if (strstr(def->os.bootloader, "grub-bhyve") != NULL) { + return virBhyveProcessBuildGrubbhyveCmd(def, priv, conn); + } else { + return virBhyveProcessBuildCustomLoaderCmd(def); + } } diff --git a/src/bhyve/bhyve_command.h b/src/bhyve/bhyve_command.h index 5b323bf..1da7f69 100644 --- a/src/bhyve/bhyve_command.h +++ b/src/bhyve/bhyve_command.h @@ -22,6 +22,7 @@ #ifndef __BHYVE_COMMAND_H__ # define __BHYVE_COMMAND_H__ +# include "bhyve_domain.h" # include "bhyve_utils.h" # include "domain_conf.h" @@ -38,7 +39,7 @@ virBhyveProcessBuildDestroyCmd(bhyveConnPtr driver, virDomainDefPtr def); virCommandPtr -virBhyveProcessBuildLoadCmd(virConnectPtr conn, - virDomainDefPtr def); +virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv); #endif /* __BHYVE_COMMAND_H__ */ diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index ecb1758..77d34a7 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -49,6 +49,11 @@ bhyveDomainObjPrivateFree(void *data) virDomainPCIAddressSetFree(priv->pciaddrs); + if (priv->grub_devicesmap_file != NULL) { + ignore_value(unlink(priv->grub_devicesmap_file)); + VIR_FREE(priv->grub_devicesmap_file); + } + VIR_FREE(priv); } diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index b8ef22a..6ecd395 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; bool persistentAddrs; + char *grub_devicesmap_file; }; extern virDomainXMLPrivateDataCallbacks virBhyveDriverPrivateDataCallbacks; diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index eb0d455..5f0ffc7 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -689,7 +689,7 @@ bhyveConnectDomainXMLToNative(virConnectPtr conn, if (bhyveDomainAssignAddresses(def, NULL) < 0) goto cleanup; - if (!(loadcmd = virBhyveProcessBuildLoadCmd(conn, def))) + if (!(loadcmd = virBhyveProcessBuildLoadCmd(conn, def, NULL))) goto cleanup; if (!(cmd = virBhyveProcessBuildBhyveCmd(conn, def, true))) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 0bbe388..8a59227 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -102,7 +102,8 @@ virBhyveProcessStart(virConnectPtr conn, virCommandPtr cmd = NULL; virCommandPtr load_cmd = NULL; bhyveConnPtr privconn = conn->privateData; - int ret = -1; + bhyveDomainObjPrivatePtr priv = vm->privateData; + int ret = -1, rc; if (virAsprintf(&logfile, "%s/%s.log", BHYVE_LOG_DIR, vm->def->name) < 0) @@ -151,7 +152,7 @@ virBhyveProcessStart(virConnectPtr conn, /* 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(conn, vm->def))) + if (!(load_cmd = virBhyveProcessBuildLoadCmd(conn, vm->def, priv))) goto cleanup; virCommandSetOutputFD(load_cmd, &logfd); virCommandSetErrorFD(load_cmd, &logfd); @@ -193,6 +194,14 @@ virBhyveProcessStart(virConnectPtr conn, ret = 0; cleanup: + if (priv->grub_devicesmap_file != NULL) { + rc = unlink(priv->grub_devicesmap_file); + if (rc < 0) + virReportSystemError(errno, _("cannot unlink file '%s'"), + priv->grub_devicesmap_file); + VIR_FREE(priv->grub_devicesmap_file); + } + if (ret < 0) { int exitstatus; /* Needed to avoid logging non-zero status */ virCommandPtr destroy_cmd; -- 1.9.3
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
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 (or a CD if one exists, under the assumption that for a VM a CD is likely an install source).
Caveat: Assumes the HDD boots from the msdos1 partition. I think this is a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload already assumes that the first disk should be booted.)
I've tested both HDD and CD boot and they seem to work.
Sponsored by: EMC / Isilon storage division
BTW, please remove these sponsored by lines if you need to post this series again. GIT commit messages should be about the change posted. For advertizing sponsorship by a company, use a personal blog post or website or some other suitable channel. 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 :|
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
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 (or a CD if one exists, under the assumption that for a VM a CD is likely an install source).
Caveat: Assumes the HDD boots from the msdos1 partition. I think this is a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload already assumes that the first disk should be booted.)
I've tested both HDD and CD boot and they seem to work.
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/drvbhyve.html.in | 94 ++++++++++++++++++++- docs/formatdomain.html.in | 4 +- src/bhyve/bhyve_command.c | 202 +++++++++++++++++++++++++++++++++++++++++----- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_domain.c | 5 ++ src/bhyve/bhyve_domain.h | 1 + src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 13 ++- 8 files changed, 298 insertions(+), 28 deletions(-)
diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in index 39afdf5..ee1317e 100644 --- a/docs/drvbhyve.html.in +++ b/docs/drvbhyve.html.in @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/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> @@ -48,10 +47,21 @@ disk device were supported per-domain. However, up to 31 PCI devices. </p>
+<p> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you +want to install from CD, put the CD device first. If not, put the root HDD +first. +</p>
FYI, the libvirt XML allows for a <boot order='NNN'/> element to be included by the user in any <disk>, <interface> or <hostdev> element. This is considered to obsolete the <boot dev="cdrom|disk|network"/> config we originally used, so that we can get fine grained control over device boot order, independant of XML format. Your patch is fine as it is, i just mention this as something you might consider adding support for in later work.
<h2><a name="usage">Guest usage / management</a></h2>
@@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p>
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader. If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem. Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access. So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt. We need to at least come up with a plan of how we'd address this problem in the medium term.
+static virCommandPtr +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv, + virConnectPtr conn) +{ + virDomainDiskDefPtr disk, cd; + virCommandPtr cmd; + size_t i; + FILE *f; + int rc; + + if (def->os.bootloaderArgs != NULL) + return virBhyveProcessBuildCustomLoaderCmd(def); + + /* 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)); + } + } + + cmd = virCommandNew(def->os.bootloader); + + VIR_DEBUG("grub-bhyve with default arguments"); + + if (priv != NULL) { + rc = virAsprintf(&priv->grub_devicesmap_file, + "%s/grub_bhyve-%s-device.map", BHYVE_STATE_DIR, + def->name); + if (rc < 0) + goto error; + + f = fopen(priv->grub_devicesmap_file, "wb"); + if (f == NULL) { + virReportSystemError(errno, _("Failed to open '%s'"), + priv->grub_devicesmap_file); + goto error; + } + + /* Grub device.map (just for boot) */ + if (disk != NULL) + fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk)); + + if (cd != NULL) + fprintf(f, "(cd) %s\n", virDomainDiskGetSource(cd)); + + if (VIR_FCLOSE(f) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + goto error; + }
As a general rule we prefer that the APIs for constructing command line args don't have side effects on system state, such as creating external files, because we want to be able to test all these code paths in the unit tests. The 'if (priv)' approach is somewhat fragile and means we don't get test coverage of the grub file format writing code. I think I'd suggest we need a way to just write the grub device.map to a 'char **' parameter we pass into this method, and bubble that all the wayy back to the top Then make the caller of virBhyveProcessBuildLoadCmd be responsible for writing it to disk, using virFileWriteStr. That way we can fully test the code in virBhyveProcessBuildLoadCmd without hitting the file writing path.
+ } + + if (cd != NULL) { + VIR_WARN("Trying to boot cd with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>");
We have a general policy that VIR_WARN should not be used for messages that are related to user API actions / choices. This is because the person who is causing this warning, is typically not going to be the person who has visibility into the libvirtd daemon log file.
+ + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "cd"); + } else { + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "hd0,msdos1"); + }
As mentioned above we have spport for per-device boot indexes, but in the absence of that, I think you should at least be honouring the traditianal <boot dev="cdrom|disk|network"> element in the XML config rather than hardcoding priority for cdrom over disks.
+ + virCommandAddArg(cmd, "--device-map"); + if (priv != NULL) + virCommandAddArg(cmd, priv->grub_devicesmap_file); + else + virCommandAddArg(cmd, "<device.map>"); + + /* 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; + + error: + virCommandFree(cmd); + return NULL; +} + +virCommandPtr +virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv) +{ + virDomainDiskDefPtr disk; + + if (def->ndisks < 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("domain should have at least one disk defined")); + return NULL; + } + + if (def->os.bootloader == NULL) { + disk = def->disks[0]; + + if (!virBhyveUsableDisk(conn, disk)) + return NULL; + + if (def->ndisks > 1) { + VIR_WARN("drvbhyve attempting to boot from device %s of multiple-" + "device configuration", virDomainDiskGetSource(disk)); + } + + return virBhyveProcessBuildBhyveloadCmd(def, disk); + } else if (strstr(def->os.bootloader, "grub-bhyve") != NULL) { + return virBhyveProcessBuildGrubbhyveCmd(def, priv, conn); + } else { + return virBhyveProcessBuildCustomLoaderCmd(def); + } }
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index ecb1758..77d34a7 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -49,6 +49,11 @@ bhyveDomainObjPrivateFree(void *data)
virDomainPCIAddressSetFree(priv->pciaddrs);
+ if (priv->grub_devicesmap_file != NULL) { + ignore_value(unlink(priv->grub_devicesmap_file)); + VIR_FREE(priv->grub_devicesmap_file); + } + VIR_FREE(priv); } diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index b8ef22a..6ecd395 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; bool persistentAddrs; + char *grub_devicesmap_file; };
I'm wondering if we need to store this filename here. If we restart libvirtd while a bhyve guest is running, then I think we loose this filename data, so we'd then miss the cleanup. Perhaps it is better if we just make the bhve guest shutdown method re-create the filename string and unconditionally unlink it, ignoring any ENOENT error. 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 :|
Daniel P. Berrange wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
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 (or a CD if one exists, under the assumption that for a VM a CD is likely an install source).
Caveat: Assumes the HDD boots from the msdos1 partition. I think this is a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload already assumes that the first disk should be booted.)
I've tested both HDD and CD boot and they seem to work.
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/drvbhyve.html.in | 94 ++++++++++++++++++++- docs/formatdomain.html.in | 4 +- src/bhyve/bhyve_command.c | 202 +++++++++++++++++++++++++++++++++++++++++----- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_domain.c | 5 ++ src/bhyve/bhyve_domain.h | 1 + src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 13 ++- 8 files changed, 298 insertions(+), 28 deletions(-)
diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in index 39afdf5..ee1317e 100644 --- a/docs/drvbhyve.html.in +++ b/docs/drvbhyve.html.in @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/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> @@ -48,10 +47,21 @@ disk device were supported per-domain. However, up to 31 PCI devices. </p>
+<p> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you +want to install from CD, put the CD device first. If not, put the root HDD +first. +</p>
FYI, the libvirt XML allows for a <boot order='NNN'/> element to be included by the user in any <disk>, <interface> or <hostdev> element.
This is considered to obsolete the <boot dev="cdrom|disk|network"/> config we originally used, so that we can get fine grained control over device boot order, independant of XML format.
Your patch is fine as it is, i just mention this as something you might consider adding support for in later work.
<h2><a name="usage">Guest usage / management</a></h2>
@@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p>
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader.
If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem.
Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access.
So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt.
We need to at least come up with a plan of how we'd address this problem in the medium term.
That is my concern as well. :( I'm wondering if we probably should just state that we're currently support VMs that don't need any interactive interactions at the boot loader step? Vaporware or not, I'm sure that solving it on the Bhyve sise (e.g. either get UEFI support or at least pass loader options to it so it could run it itself) is a much better solution than development of the workarounds in libvirt...
+static virCommandPtr +virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv, + virConnectPtr conn) +{ + virDomainDiskDefPtr disk, cd; + virCommandPtr cmd; + size_t i; + FILE *f; + int rc; + + if (def->os.bootloaderArgs != NULL) + return virBhyveProcessBuildCustomLoaderCmd(def); + + /* 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)); + } + } + + cmd = virCommandNew(def->os.bootloader); + + VIR_DEBUG("grub-bhyve with default arguments"); + + if (priv != NULL) { + rc = virAsprintf(&priv->grub_devicesmap_file, + "%s/grub_bhyve-%s-device.map", BHYVE_STATE_DIR, + def->name); + if (rc < 0) + goto error; + + f = fopen(priv->grub_devicesmap_file, "wb"); + if (f == NULL) { + virReportSystemError(errno, _("Failed to open '%s'"), + priv->grub_devicesmap_file); + goto error; + } + + /* Grub device.map (just for boot) */ + if (disk != NULL) + fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk)); + + if (cd != NULL) + fprintf(f, "(cd) %s\n", virDomainDiskGetSource(cd)); + + if (VIR_FCLOSE(f) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + goto error; + }
As a general rule we prefer that the APIs for constructing command line args don't have side effects on system state, such as creating external files, because we want to be able to test all these code paths in the unit tests.
The 'if (priv)' approach is somewhat fragile and means we don't get test coverage of the grub file format writing code.
I think I'd suggest we need a way to just write the grub device.map to a 'char **' parameter we pass into this method, and bubble that all the wayy back to the top
Then make the caller of virBhyveProcessBuildLoadCmd be responsible for writing it to disk, using virFileWriteStr. That way we can fully test the code in virBhyveProcessBuildLoadCmd without hitting the file writing path.
+ } + + if (cd != NULL) { + VIR_WARN("Trying to boot cd with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>");
We have a general policy that VIR_WARN should not be used for messages that are related to user API actions / choices. This is because the person who is causing this warning, is typically not going to be the person who has visibility into the libvirtd daemon log file.
+ + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "cd"); + } else { + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "hd0,msdos1"); + }
As mentioned above we have spport for per-device boot indexes, but in the absence of that, I think you should at least be honouring the traditianal <boot dev="cdrom|disk|network"> element in the XML config rather than hardcoding priority for cdrom over disks.
+ + virCommandAddArg(cmd, "--device-map"); + if (priv != NULL) + virCommandAddArg(cmd, priv->grub_devicesmap_file); + else + virCommandAddArg(cmd, "<device.map>"); + + /* 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; + + error: + virCommandFree(cmd); + return NULL; +} + +virCommandPtr +virBhyveProcessBuildLoadCmd(virConnectPtr conn, virDomainDefPtr def, + bhyveDomainObjPrivatePtr priv) +{ + virDomainDiskDefPtr disk; + + if (def->ndisks < 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("domain should have at least one disk defined")); + return NULL; + } + + if (def->os.bootloader == NULL) { + disk = def->disks[0]; + + if (!virBhyveUsableDisk(conn, disk)) + return NULL; + + if (def->ndisks > 1) { + VIR_WARN("drvbhyve attempting to boot from device %s of multiple-" + "device configuration", virDomainDiskGetSource(disk)); + } + + return virBhyveProcessBuildBhyveloadCmd(def, disk); + } else if (strstr(def->os.bootloader, "grub-bhyve") != NULL) { + return virBhyveProcessBuildGrubbhyveCmd(def, priv, conn); + } else { + return virBhyveProcessBuildCustomLoaderCmd(def); + } }
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index ecb1758..77d34a7 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -49,6 +49,11 @@ bhyveDomainObjPrivateFree(void *data)
virDomainPCIAddressSetFree(priv->pciaddrs);
+ if (priv->grub_devicesmap_file != NULL) { + ignore_value(unlink(priv->grub_devicesmap_file)); + VIR_FREE(priv->grub_devicesmap_file); + } + VIR_FREE(priv); } diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index b8ef22a..6ecd395 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; bool persistentAddrs; + char *grub_devicesmap_file; };
I'm wondering if we need to store this filename here. If we restart libvirtd while a bhyve guest is running, then I think we loose this filename data, so we'd then miss the cleanup.
Perhaps it is better if we just make the bhve guest shutdown method re-create the filename string and unconditionally unlink it, ignoring any ENOENT error.
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 :|
Roman Bogorodskiy
On Tue, Oct 28, 2014 at 06:01:51PM +0300, Roman Bogorodskiy wrote:
Daniel P. Berrange wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
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 (or a CD if one exists, under the assumption that for a VM a CD is likely an install source).
Caveat: Assumes the HDD boots from the msdos1 partition. I think this is a pretty reasonable assumption for a VM. (DrvBhyve with Bhyveload already assumes that the first disk should be booted.)
I've tested both HDD and CD boot and they seem to work.
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/drvbhyve.html.in | 94 ++++++++++++++++++++- docs/formatdomain.html.in | 4 +- src/bhyve/bhyve_command.c | 202 +++++++++++++++++++++++++++++++++++++++++----- src/bhyve/bhyve_command.h | 5 +- src/bhyve/bhyve_domain.c | 5 ++ src/bhyve/bhyve_domain.h | 1 + src/bhyve/bhyve_driver.c | 2 +- src/bhyve/bhyve_process.c | 13 ++- 8 files changed, 298 insertions(+), 28 deletions(-)
diff --git a/docs/drvbhyve.html.in b/docs/drvbhyve.html.in index 39afdf5..ee1317e 100644 --- a/docs/drvbhyve.html.in +++ b/docs/drvbhyve.html.in @@ -37,8 +37,7 @@ bhyve+ssh://root@example.com/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> @@ -48,10 +47,21 @@ disk device were supported per-domain. However, up to 31 PCI devices. </p>
+<p> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you +want to install from CD, put the CD device first. If not, put the root HDD +first. +</p>
FYI, the libvirt XML allows for a <boot order='NNN'/> element to be included by the user in any <disk>, <interface> or <hostdev> element.
This is considered to obsolete the <boot dev="cdrom|disk|network"/> config we originally used, so that we can get fine grained control over device boot order, independant of XML format.
Your patch is fine as it is, i just mention this as something you might consider adding support for in later work.
<h2><a name="usage">Guest usage / management</a></h2>
@@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p>
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader.
If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem.
Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access.
So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt.
We need to at least come up with a plan of how we'd address this problem in the medium term.
That is my concern as well. :( I'm wondering if we probably should just state that we're currently support VMs that don't need any interactive interactions at the boot loader step?
Vaporware or not, I'm sure that solving it on the Bhyve sise (e.g. either get UEFI support or at least pass loader options to it so it could run it itself) is a much better solution than development of the workarounds in libvirt...
Maybe it suffices to just say that libvirt does not officially support interactive bootloaders. Don't try to block any specific configurations. People can use the out-of-band hack if they really want to, but we make no promises about future compatibility or support of this hack. So if it breaks in a upgrade they get to keep both pieces. Then long term just focus on fully interactive BIOS running guestside. 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 :|
On Tue, Oct 28, 2014 at 11:14 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Tue, Oct 28, 2014 at 06:01:51PM +0300, Roman Bogorodskiy wrote:
Daniel P. Berrange wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader.
If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem.
Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access.
So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt.
We need to at least come up with a plan of how we'd address this problem in the medium term.
That is my concern as well. :( I'm wondering if we probably should just state that we're currently support VMs that don't need any interactive interactions at the boot loader step?
Vaporware or not, I'm sure that solving it on the Bhyve sise (e.g. either get UEFI support or at least pass loader options to it so it could run it itself) is a much better solution than development of the workarounds in libvirt...
Maybe it suffices to just say that libvirt does not officially support interactive bootloaders. Don't try to block any specific configurations. People can use the out-of-band hack if they really want to, but we make no promises about future compatibility or support of this hack. So if it breaks in a upgrade they get to keep both pieces. Then long term just focus on fully interactive BIOS running guestside.
+1 from me (so long as the out-of-band hack isn't explicitly blocked). Best, Conrad
On Tue, Oct 28, 2014 at 11:01 AM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
Daniel P. Berrange wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader.
If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem.
Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access.
So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt.
We need to at least come up with a plan of how we'd address this problem in the medium term.
That is my concern as well. :( I'm wondering if we probably should just state that we're currently support VMs that don't need any interactive interactions at the boot loader step?
Sure.
Vaporware or not, I'm sure that solving it on the Bhyve sise (e.g. either get UEFI support or at least pass loader options to it so it could run it itself) is a much better solution than development of the workarounds in libvirt...
Yes. I can draft a patch for bhyve to run the loader itself, but I fear they might reject it due to "UEFI/BIOS support is coming." Anyway, I think that's a bit orthogonal to this change. We already block / fall over in the same way if bhyveload pauses and waits for user input. Thanks, Conrad
On Tue, Oct 28, 2014 at 10:39 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Oct 27, 2014 at 10:37:36AM -0400, Conrad Meyer wrote:
+<p> +Note: the Bhyve driver in libvirt will boot whichever device is first. If you +want to install from CD, put the CD device first. If not, put the root HDD +first. +</p>
FYI, the libvirt XML allows for a <boot order='NNN'/> element to be included by the user in any <disk>, <interface> or <hostdev> element.
This is considered to obsolete the <boot dev="cdrom|disk|network"/> config we originally used, so that we can get fine grained control over device boot order, independant of XML format.
Your patch is fine as it is, i just mention this as something you might consider adding support for in later work.
Right, that would be nice to fix eventually. libvirt's bhyve driver doesn't support boot order at all yet (it just picks 'domdef->drives[0]'). Fixing that is orthogonal to adding grub-bhyve as a loader option, IMO.
<h2><a name="usage">Guest usage / management</a></h2>
@@ -119,6 +177,14 @@ to let a guest boot or start a guest using:</p>
<pre>start --console domname</pre>
+<p><b>NB:</b> An interactive bootloader will keep the domain from starting (and +thus <code>virsh console</code> or <code>start --console</code>) until the +console is opened by a client. To select a boot option and allow the domain to +finish starting, one must use an alternative terminal client to connect to the +null modem device. One example is:</p>
Can you clarify what you mean by this. It seems like this is saying that the 'virDomainCreate' API (virsh start GUEST command) will not return controll to the caller until you've interacted with the bootloader.
For GRUB configurations that do not automatically boot something without user interaction, you are correct.
If correct, I don't think this is a very satisfactory solution. There should not be any requirement to interact with the guest domain until after the virDomainCreate API call completes successfully. If succesfully booting requires that you go via an out-of-band channel to interact with the bootloader that's even more of a problem.
I agree, however — this is the status quo of libvirt-bhyve. This patchset doesn't make it any worse than it already is.
Because libvirt has an RPC based mechanism for API access, we need to always bear in mind that the application/user talking to libvirt drivers is either local but unprivileged, or even entirely remote from the hypervisor node. The libvirt built-in console tunnels over our RPC channel to provide apps access.
So if my understanding is correct, this limitation you describe is going to prevent use of this kind of setup from most apps using libvirt.
I think most apps probably have automatic boot in their GRUB configuration. But for anything that requires user input before GRUB will load the OS, you are correct.
We need to at least come up with a plan of how we'd address this problem in the medium term.
I think the medium- or long-term plan is to not require an external bootloader at all, this is sort of an interim solution.
+ if (priv != NULL) { + rc = virAsprintf(&priv->grub_devicesmap_file, + "%s/grub_bhyve-%s-device.map", BHYVE_STATE_DIR, + def->name); + if (rc < 0) + goto error; + + f = fopen(priv->grub_devicesmap_file, "wb"); + if (f == NULL) { + virReportSystemError(errno, _("Failed to open '%s'"), + priv->grub_devicesmap_file); + goto error; + } + + /* Grub device.map (just for boot) */ + if (disk != NULL) + fprintf(f, "(hd0) %s\n", virDomainDiskGetSource(disk)); + + if (cd != NULL) + fprintf(f, "(cd) %s\n", virDomainDiskGetSource(cd)); + + if (VIR_FCLOSE(f) < 0) { + virReportSystemError(errno, "%s", _("failed to close file")); + goto error; + }
As a general rule we prefer that the APIs for constructing command line args don't have side effects on system state, such as creating external files, because we want to be able to test all these code paths in the unit tests.
Definitely.
The 'if (priv)' approach is somewhat fragile and means we don't get test coverage of the grub file format writing code.
I think I'd suggest we need a way to just write the grub device.map to a 'char **' parameter we pass into this method, and bubble that all the wayy back to the top
Then make the caller of virBhyveProcessBuildLoadCmd be responsible for writing it to disk, using virFileWriteStr. That way we can fully test the code in virBhyveProcessBuildLoadCmd without hitting the file writing path.
I have some reservations about this (additional complexity, not a lot of value IMO), but it can be done if you want it.
+ } + + if (cd != NULL) { + VIR_WARN("Trying to boot cd with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>");
We have a general policy that VIR_WARN should not be used for messages that are related to user API actions / choices. This is because the person who is causing this warning, is typically not going to be the person who has visibility into the libvirtd daemon log file.
Ack. Will fix.
+ + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "cd"); + } else { + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "hd0,msdos1"); + }
As mentioned above we have spport for per-device boot indexes, but in the absence of that, I think you should at least be honouring the traditianal <boot dev="cdrom|disk|network"> element in the XML config rather than hardcoding priority for cdrom over disks.
I think that's an orthogonal improvement (bhyveload currently doesn't support ordering at all). This patch set is already getting large, can this improvement wait?
diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index b8ef22a..6ecd395 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; bool persistentAddrs; + char *grub_devicesmap_file; };
I'm wondering if we need to store this filename here. If we restart libvirtd while a bhyve guest is running, then I think we loose this filename data, so we'd then miss the cleanup.
Perhaps it is better if we just make the bhve guest shutdown method re-create the filename string and unconditionally unlink it, ignoring any ENOENT error.
I think we can probably just remove it from the object if we're returning the string contents out to the caller — the file is very short-lived; we only need it in the routine that launches the loader, synchronously waits for it to complete, and then asynchronously launches bhyve itself. Thanks for reviewing. Conrad
On Tue, Oct 28, 2014 at 11:11:18AM -0400, Conrad Meyer wrote:
On Tue, Oct 28, 2014 at 10:39 AM, Daniel P. Berrange
+ + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "cd"); + } else { + VIR_WARN("Trying to boot hd0,msdos1 with grub-bhyve. If this is " + "not what you wanted, specify <bootloader_args>"); + + virCommandAddArg(cmd, "--root"); + virCommandAddArg(cmd, "hd0,msdos1"); + }
As mentioned above we have spport for per-device boot indexes, but in the absence of that, I think you should at least be honouring the traditianal <boot dev="cdrom|disk|network"> element in the XML config rather than hardcoding priority for cdrom over disks.
I think that's an orthogonal improvement (bhyveload currently doesn't support ordering at all). This patch set is already getting large, can this improvement wait?
Ok, you can do that separately.
diff --git a/src/bhyve/bhyve_domain.h b/src/bhyve/bhyve_domain.h index b8ef22a..6ecd395 100644 --- a/src/bhyve/bhyve_domain.h +++ b/src/bhyve/bhyve_domain.h @@ -31,6 +31,7 @@ typedef bhyveDomainObjPrivate *bhyveDomainObjPrivatePtr; struct _bhyveDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; bool persistentAddrs; + char *grub_devicesmap_file; };
I'm wondering if we need to store this filename here. If we restart libvirtd while a bhyve guest is running, then I think we loose this filename data, so we'd then miss the cleanup.
Perhaps it is better if we just make the bhve guest shutdown method re-create the filename string and unconditionally unlink it, ignoring any ENOENT error.
I think we can probably just remove it from the object if we're returning the string contents out to the caller — the file is very short-lived; we only need it in the routine that launches the loader, synchronously waits for it to complete, and then asynchronously launches bhyve itself.
Ok that sounds fine. 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 :|
Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- .../bhyvexml2argv-acpiapic.ldargs | 1 + tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-console.ldargs | 1 + .../bhyvexml2argv-disk-cdrom.ldargs | 1 + .../bhyvexml2argv-disk-virtio.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-serial.ldargs | 1 + tests/bhyvexml2argvtest.c | 36 ++++++++++++++++++---- 8 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs new file mode 100644 index 0000000..0eb3cc9 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/cdrom.iso bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs new file mode 100644 index 0000000..215d65f --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -m 214 -d /tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index b9be378..db45ae5 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -15,14 +15,15 @@ static bhyveConn driver; static int testCompareXMLToArgvFiles(const char *xml, - const char *cmdline) + const char *cmdline, + const char *ldcmdline) { - char *expectargv = NULL; + char *expectargv = NULL, *expectld = NULL; int len; - char *actualargv = NULL; + char *actualargv = NULL, *actualld = NULL; virDomainDefPtr vmdef = NULL; virDomainObj vm; - virCommandPtr cmd = NULL; + virCommandPtr cmd = NULL, ldcmd = NULL; virConnectPtr conn; int ret = -1; @@ -42,6 +43,12 @@ static int testCompareXMLToArgvFiles(const char *xml, if (!(actualargv = virCommandToString(cmd))) goto out; + if (!(ldcmd = virBhyveProcessBuildLoadCmd(conn, vmdef, NULL))) + goto out; + + if (!(actualld = virCommandToString(ldcmd))) + goto out; + len = virtTestLoadFile(cmdline, &expectargv); if (len < 0) goto out; @@ -49,17 +56,32 @@ static int testCompareXMLToArgvFiles(const char *xml, if (len && expectargv[len - 1] == '\n') expectargv[len - 1] = '\0'; + len = virtTestLoadFile(ldcmdline, &expectld); + if (len < 0) + goto out; + + if (len && expectld[len - 1] == '\n') + expectld[len - 1] = '\0'; + if (STRNEQ(expectargv, actualargv)) { virtTestDifference(stderr, expectargv, actualargv); goto out; } + if (STRNEQ(expectld, actualld)) { + virtTestDifference(stderr, expectld, actualld); + goto out; + } + ret = 0; out: VIR_FREE(expectargv); + VIR_FREE(expectld); VIR_FREE(actualargv); + VIR_FREE(actualld); virCommandFree(cmd); + virCommandFree(ldcmd); virDomainDefFree(vmdef); return ret; } @@ -70,15 +92,17 @@ testCompareXMLToArgvHelper(const void *data) int ret = -1; const char *name = data; char *xml = NULL; - char *args = NULL; + char *args = NULL, *ldargs = NULL; if (virAsprintf(&xml, "%s/bhyvexml2argvdata/bhyvexml2argv-%s.xml", abs_srcdir, name) < 0 || virAsprintf(&args, "%s/bhyvexml2argvdata/bhyvexml2argv-%s.args", + abs_srcdir, name) < 0 || + virAsprintf(&ldargs, "%s/bhyvexml2argvdata/bhyvexml2argv-%s.ldargs", abs_srcdir, name) < 0) goto cleanup; - ret = testCompareXMLToArgvFiles(xml, args); + ret = testCompareXMLToArgvFiles(xml, args, ldargs); cleanup: VIR_FREE(xml); -- 1.9.3
On Mon, Oct 27, 2014 at 10:37:37AM -0400, Conrad Meyer wrote:
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- .../bhyvexml2argv-acpiapic.ldargs | 1 + tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-console.ldargs | 1 + .../bhyvexml2argv-disk-cdrom.ldargs | 1 + .../bhyvexml2argv-disk-virtio.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs | 1 + .../bhyvexml2argvdata/bhyvexml2argv-serial.ldargs | 1 + tests/bhyvexml2argvtest.c | 36 ++++++++++++++++++---- 8 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-acpiapic.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-base.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-console.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-virtio.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial.ldargs
ACK, good addition. 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 :|
Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/schemas/domaincommon.rng | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 20d81ae..1c444e0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -213,7 +213,7 @@ <choice> <group> <optional> - <ref name="bootloader"/> + <ref name="xenbootloader"/> </optional> <element name="os"> <ref name="ostypexen"/> @@ -221,7 +221,7 @@ </element> </group> <group> - <ref name="bootloader"/> + <ref name="xenbootloader"/> <optional> <element name="os"> <ref name="ostypexen"/> @@ -234,6 +234,9 @@ </choice> </define> <define name="oshvm"> + <optional> + <ref name="bhyvebootloader"/> + </optional> <element name="os"> <ref name="ostypehvm"/> <interleave> @@ -1053,17 +1056,33 @@ binary or script used to extract the data from the first disk device. --> <define name="bootloader"> + <element name="bootloader"> + <choice> + <ref name="absFilePath"/> + <empty/> + </choice> + </element> + </define> + <define name="bootloader_args"> + <element name="bootloader_args"> + <text/> + </element> + </define> + <define name="xenbootloader"> <interleave> - <element name="bootloader"> - <choice> - <ref name="absFilePath"/> - <empty/> - </choice> - </element> + <ref name="bootloader"/> <optional> - <element name="bootloader_args"> - <text/> - </element> + <ref name="bootloader_args"/> + </optional> + </interleave> + </define> + <define name="bhyvebootloader"> + <interleave> + <optional> + <ref name="bootloader"/> + </optional> + <optional> + <ref name="bootloader_args"/> </optional> </interleave> </define> -- 1.9.3
On Mon, Oct 27, 2014 at 10:37:38AM -0400, Conrad Meyer wrote:
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- docs/schemas/domaincommon.rng | 41 ++++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 20d81ae..1c444e0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -213,7 +213,7 @@ <choice> <group> <optional> - <ref name="bootloader"/> + <ref name="xenbootloader"/> </optional> <element name="os"> <ref name="ostypexen"/> @@ -221,7 +221,7 @@ </element> </group> <group> - <ref name="bootloader"/> + <ref name="xenbootloader"/> <optional> <element name="os"> <ref name="ostypexen"/> @@ -234,6 +234,9 @@ </choice> </define> <define name="oshvm"> + <optional> + <ref name="bhyvebootloader"/> + </optional> <element name="os"> <ref name="ostypehvm"/> <interleave> @@ -1053,17 +1056,33 @@ binary or script used to extract the data from the first disk device. --> <define name="bootloader"> + <element name="bootloader"> + <choice> + <ref name="absFilePath"/> + <empty/> + </choice> + </element> + </define> + <define name="bootloader_args"> + <element name="bootloader_args"> + <text/> + </element> + </define> + <define name="xenbootloader"> <interleave> - <element name="bootloader"> - <choice> - <ref name="absFilePath"/> - <empty/> - </choice> - </element> + <ref name="bootloader"/> <optional> - <element name="bootloader_args"> - <text/> - </element> + <ref name="bootloader_args"/> + </optional> + </interleave> + </define> + <define name="bhyvebootloader"> + <interleave> + <optional> + <ref name="bootloader"/> + </optional> + <optional> + <ref name="bootloader_args"/> </optional> </interleave> </define>
I understand you created separate bhyvebootloader vs xenbootloader schema rules, so that the Xen case can keep <bootloader> as compulsory for use when <bootloader_args> is set, while for bhyve you allow <bootloader_args> without any corresponding <bootloader>. The actual XML parser meanwhile, always allows <bootloader_args> even when <bootloader> is not set. So I think you don't really need to have the complexity of making the schema provide different rules for xen vs bhyve. Lets just stay simple and keep a single <define name="bootloader"/>element and make the <bootloader> optional to match what the parser actually does. 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 :|
On Tue, Oct 28, 2014 at 10:49 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
I understand you created separate bhyvebootloader vs xenbootloader schema rules, so that the Xen case can keep <bootloader> as compulsory for use when <bootloader_args> is set, while for bhyve you allow <bootloader_args> without any corresponding <bootloader>.
The actual XML parser meanwhile, always allows <bootloader_args> even when <bootloader> is not set. So I think you don't really need to have the complexity of making the schema provide different rules for xen vs bhyve.
Lets just stay simple and keep a single <define name="bootloader"/>element and make the <bootloader> optional to match what the parser actually does.
Sure, sounds reasonable to me. I'll fix it in the next submission. Thanks, Conrad
Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- .../bhyvexml2argv-bhyveload-explicitargs.args | 3 +++ .../bhyvexml2argv-bhyveload-explicitargs.ldargs | 1 + .../bhyvexml2argv-bhyveload-explicitargs.xml | 23 +++++++++++++++++++++ .../bhyvexml2argv-custom-loader.args | 3 +++ .../bhyvexml2argv-custom-loader.ldargs | 1 + .../bhyvexml2argv-custom-loader.xml | 24 ++++++++++++++++++++++ .../bhyvexml2argv-disk-cdrom-grub.args | 3 +++ .../bhyvexml2argv-disk-cdrom-grub.ldargs | 2 ++ .../bhyvexml2argv-disk-cdrom-grub.xml | 23 +++++++++++++++++++++ .../bhyvexml2argv-grub-defaults.args | 3 +++ .../bhyvexml2argv-grub-defaults.ldargs | 2 ++ .../bhyvexml2argv-grub-defaults.xml | 23 +++++++++++++++++++++ tests/bhyvexml2argvtest.c | 4 ++++ 13 files changed, 115 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args new file mode 100644 index 0000000..4122e62 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args @@ -0,0 +1,3 @@ +/usr/sbin/bhyve -c 1 -m 214 -H -P -s 0:0,hostbridge \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-hd,/tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs new file mode 100644 index 0000000..a3f58f0 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs @@ -0,0 +1 @@ +/usr/sbin/bhyveload -X -Y -Z diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml new file mode 100644 index 0000000..65823cd --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml @@ -0,0 +1,23 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <bootloader_args>-X -Y -Z</bootloader_args> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args new file mode 100644 index 0000000..4122e62 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args @@ -0,0 +1,3 @@ +/usr/sbin/bhyve -c 1 -m 214 -H -P -s 0:0,hostbridge \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-hd,/tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs new file mode 100644 index 0000000..100e163 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs @@ -0,0 +1 @@ +/fizz_buzz_bazz -X -Y -Z diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml new file mode 100644 index 0000000..e9e0d5e --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml @@ -0,0 +1,24 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <bootloader>/fizz_buzz_bazz</bootloader> + <bootloader_args>-X -Y -Z</bootloader_args> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args new file mode 100644 index 0000000..eb38969 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args @@ -0,0 +1,3 @@ +/usr/sbin/bhyve -c 1 -m 214 -H -P -s 0:0,hostbridge \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-cd,/tmp/cdrom.iso bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs new file mode 100644 index 0000000..d9161ba --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root cd --device-map '<device.map>' --memory 214 \ +bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml new file mode 100644 index 0000000..00708e6 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml @@ -0,0 +1,23 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file' device='cdrom'> + <driver name='file' type='raw'/> + <source file='/tmp/cdrom.iso'/> + <target dev='hdc' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args new file mode 100644 index 0000000..4122e62 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args @@ -0,0 +1,3 @@ +/usr/sbin/bhyve -c 1 -m 214 -H -P -s 0:0,hostbridge \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-hd,/tmp/freebsd.img bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs new file mode 100644 index 0000000..91c15ce --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root hd0,msdos1 --device-map '<device.map>' \ +--memory 214 bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml new file mode 100644 index 0000000..27b7fd9 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml @@ -0,0 +1,23 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + </devices> +</domain> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index db45ae5..0f8582e 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -136,6 +136,10 @@ mymain(void) DO_TEST("macaddr"); DO_TEST("serial"); DO_TEST("console"); + DO_TEST("grub-defaults"); + DO_TEST("bhyveload-explicitargs"); + DO_TEST("custom-loader"); + DO_TEST("disk-cdrom-grub"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 1.9.3
On Mon, Oct 27, 2014 at 10:37:39AM -0400, Conrad Meyer wrote:
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- .../bhyvexml2argv-bhyveload-explicitargs.args | 3 +++ .../bhyvexml2argv-bhyveload-explicitargs.ldargs | 1 + .../bhyvexml2argv-bhyveload-explicitargs.xml | 23 +++++++++++++++++++++ .../bhyvexml2argv-custom-loader.args | 3 +++ .../bhyvexml2argv-custom-loader.ldargs | 1 + .../bhyvexml2argv-custom-loader.xml | 24 ++++++++++++++++++++++ .../bhyvexml2argv-disk-cdrom-grub.args | 3 +++ .../bhyvexml2argv-disk-cdrom-grub.ldargs | 2 ++ .../bhyvexml2argv-disk-cdrom-grub.xml | 23 +++++++++++++++++++++ .../bhyvexml2argv-grub-defaults.args | 3 +++ .../bhyvexml2argv-grub-defaults.ldargs | 2 ++ .../bhyvexml2argv-grub-defaults.xml | 23 +++++++++++++++++++++ tests/bhyvexml2argvtest.c | 4 ++++ 13 files changed, 115 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-bhyveload-explicitargs.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-custom-loader.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.xml create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-grub-defaults.xml
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs new file mode 100644 index 0000000..d9161ba --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root cd --device-map '<device.map>' --memory 214 \ +bhyve
Ah ha, I thought my command about device.map would become relevant :-) We probably want a 3rd expected data file '.devmap' containing the expected device map contents to compare against. 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 :|
On Tue, Oct 28, 2014 at 10:55 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Oct 27, 2014 at 10:37:39AM -0400, Conrad Meyer wrote:
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs new file mode 100644 index 0000000..d9161ba --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root cd --device-map '<device.map>' --memory 214 \ +bhyve
Ah ha, I thought my command about device.map would become relevant :-)
We probably want a 3rd expected data file '.devmap' containing the expected device map contents to compare against.
This can be done, sure. (IMO GRUB only needs to know about the boot device, be it CD or HDD, so this configuration will never be very exciting / surprising.) Thanks, Conrad
On Tue, Oct 28, 2014 at 11:16:52AM -0400, Conrad Meyer wrote:
On Tue, Oct 28, 2014 at 10:55 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Mon, Oct 27, 2014 at 10:37:39AM -0400, Conrad Meyer wrote:
diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs new file mode 100644 index 0000000..d9161ba --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-disk-cdrom-grub.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root cd --device-map '<device.map>' --memory 214 \ +bhyve
Ah ha, I thought my command about device.map would become relevant :-)
We probably want a 3rd expected data file '.devmap' containing the expected device map contents to compare against.
This can be done, sure. (IMO GRUB only needs to know about the boot device, be it CD or HDD, so this configuration will never be very exciting / surprising.)
If it is tiny, then you could also just avoid the extrenal .devmap file and do a plain STREQ() in the bhyvexml2argvtest.c file in the relevant place. 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 :|
This enables booting interactive GRUB menus (e.g. install CDs) with libvirt-bhyve. Caveat: A terminal other than the '--console' option to 'virsh start' (e.g. 'cu -l /dev/nmdm0B -s 115200') must be used to connect to grub-bhyve because the bhyve loader path is synchronous and must occur before the VM actually starts. Changing the bhyveProcessStart logic around to accommodate '--console' for interactive loader use seems like a significant project and probably not worth it, if UEFI/BIOS support for bhyve is "coming soon." Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- src/bhyve/bhyve_command.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 01f1795..07d209e 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -468,6 +468,21 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, virCommandAddArgFormat(cmd, "%llu", VIR_DIV_UP(def->mem.max_balloon, 1024)); + if (def->nserials > 0) { + virDomainChrDefPtr chr; + + chr = def->serials[0]; + + if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only nmdm console types are supported")); + return NULL; + } + + virCommandAddArg(cmd, "--cons-dev"); + virCommandAddArg(cmd, chr->source.data.file.path); + } + /* VM name */ virCommandAddArg(cmd, def->name); -- 1.9.3
Conrad Meyer wrote:
This enables booting interactive GRUB menus (e.g. install CDs) with libvirt-bhyve.
Caveat: A terminal other than the '--console' option to 'virsh start' (e.g. 'cu -l /dev/nmdm0B -s 115200') must be used to connect to grub-bhyve because the bhyve loader path is synchronous and must occur before the VM actually starts.
Could you please elaborate on that? It's not the obvious what's the limitation... Without being able to connect using virsh console, it's not very convenient for user to connect to the console (esp. if he runs virsh over a remote libvirt).
Changing the bhyveProcessStart logic around to accommodate '--console' for interactive loader use seems like a significant project and probably not worth it, if UEFI/BIOS support for bhyve is "coming soon."
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- src/bhyve/bhyve_command.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 01f1795..07d209e 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -468,6 +468,21 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, virCommandAddArgFormat(cmd, "%llu", VIR_DIV_UP(def->mem.max_balloon, 1024));
+ if (def->nserials > 0) { + virDomainChrDefPtr chr; + + chr = def->serials[0]; + + if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only nmdm console types are supported")); + return NULL; + } + + virCommandAddArg(cmd, "--cons-dev");
IMHO, one more thing worth to do is probe if the supplied grub-bhyve has this '--cons-dev' argument (e.g. check if it's there in its --help output). Otherwise, for users running an older grub-bhyve will get a weird error that they will not know how to fix. Other option is to check required version of grub-bhyve using autotools, but I guess it's not good because users will have to rebuild libvirt if they want to update bhyve-grub.
+ virCommandAddArg(cmd, chr->source.data.file.path); + } + /* VM name */ virCommandAddArg(cmd, def->name);
-- 1.9.3
Roman Bogorodskiy
On Tue, Oct 28, 2014 at 7:53 AM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
Conrad Meyer wrote:
This enables booting interactive GRUB menus (e.g. install CDs) with libvirt-bhyve.
Caveat: A terminal other than the '--console' option to 'virsh start' (e.g. 'cu -l /dev/nmdm0B -s 115200') must be used to connect to grub-bhyve because the bhyve loader path is synchronous and must occur before the VM actually starts.
Could you please elaborate on that?
Sure.
It's not the obvious what's the limitation...
The limitation is in virBhyveProcessStart and bhyveload / bhyve itself. The problem is that we cannot start bhyve until the loader has exited. So in BhyveProcessStart, we run the loader synchronously, then run bhyve asynchronously when the loader completes. Finally when BhyveProcessStart returns and sets the domain as started, 'virsh console' and 'virsh start --console' connect and start working. To allow 'virsh start --console', etc, I believe we'd have to start bhyveload / GRUB asynchronously in BhyveProcessStart, mark the domain as started, handle the asynchronous termination of bhyveload / GRUB, start bhyve... it sounds very painful. I suppose it would be a lot easier / cleaner if the bhyve binary itself learned how to start the bootloader (even if we had to explicitly give it the executable and arguments).
Without being able to connect using virsh console, it's not very convenient for user to connect to the console (esp. if he runs virsh over a remote libvirt).
Agreed. It's not perfect, but is an incremental improvement. I think probably most VMs should be configured to boot without getting stuck in the bootloader.
Changing the bhyveProcessStart logic around to accommodate '--console' for interactive loader use seems like a significant project and probably not worth it, if UEFI/BIOS support for bhyve is "coming soon."
Sponsored by: EMC / Isilon storage division
Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- src/bhyve/bhyve_command.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 01f1795..07d209e 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -468,6 +468,21 @@ virBhyveProcessBuildGrubbhyveCmd(virDomainDefPtr def, virCommandAddArgFormat(cmd, "%llu", VIR_DIV_UP(def->mem.max_balloon, 1024));
+ if (def->nserials > 0) { + virDomainChrDefPtr chr; + + chr = def->serials[0]; + + if (chr->source.type != VIR_DOMAIN_CHR_TYPE_NMDM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only nmdm console types are supported")); + return NULL; + } + + virCommandAddArg(cmd, "--cons-dev");
IMHO, one more thing worth to do is probe if the supplied grub-bhyve has this '--cons-dev' argument (e.g. check if it's there in its --help output). Otherwise, for users running an older grub-bhyve will get a weird error that they will not know how to fix.
I think there are few enough bhyve-grub and libvirt users at this point, and the experience is already broken enough, that we can just document in release notes that libvirt-1.x.y Bhyve support needs grub2-bhyve >= 808fa4f1b1 and ensure packagers and porters know to get the run-time dependency correct.
Other option is to check required version of grub-bhyve using autotools, but I guess it's not good because users will have to rebuild libvirt if they want to update bhyve-grub.
Hm, why would they have to rebuild? Thanks, Conrad
Sponsored by: EMC / Isilon storage division Signed-off-by: Conrad Meyer <conrad.meyer@isilon.com> --- .../bhyvexml2argv-serial-grub.args | 4 ++++ .../bhyvexml2argv-serial-grub.ldargs | 2 ++ .../bhyvexml2argv-serial-grub.xml | 26 ++++++++++++++++++++++ tests/bhyvexml2argvtest.c | 1 + 4 files changed, 33 insertions(+) create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.args create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.ldargs create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.xml diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.args b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.args new file mode 100644 index 0000000..df50290 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.args @@ -0,0 +1,4 @@ +/usr/sbin/bhyve -c 1 -m 214 -H -P -s 0:0,hostbridge \ +-s 3:0,virtio-net,faketapdev,mac=52:54:00:00:00:00 \ +-s 2:0,ahci-hd,/tmp/freebsd.img \ +-s 1,lpc -l com1,/dev/nmdm0A bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.ldargs b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.ldargs new file mode 100644 index 0000000..5645097 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.ldargs @@ -0,0 +1,2 @@ +/usr/local/sbin/grub-bhyve --root hd0,msdos1 --device-map '<device.map>' \ +--memory 214 --cons-dev /dev/nmdm0A bhyve diff --git a/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.xml b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.xml new file mode 100644 index 0000000..8c451f7 --- /dev/null +++ b/tests/bhyvexml2argvdata/bhyvexml2argv-serial-grub.xml @@ -0,0 +1,26 @@ +<domain type='bhyve'> + <name>bhyve</name> + <uuid>df3be7e7-a104-11e3-aeb0-50e5492bd3dc</uuid> + <memory>219136</memory> + <vcpu>1</vcpu> + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <os> + <type>hvm</type> + </os> + <devices> + <disk type='file'> + <driver name='file' type='raw'/> + <source file='/tmp/freebsd.img'/> + <target dev='hda' bus='sata'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </disk> + <interface type='bridge'> + <model type='virtio'/> + <source bridge="virbr0"/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </interface> + <serial type='nmdm'> + <source master='/dev/nmdm0A' slave='/dev/nmdm0B'/> + </serial> + </devices> +</domain> diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c index 0f8582e..d4d2133 100644 --- a/tests/bhyvexml2argvtest.c +++ b/tests/bhyvexml2argvtest.c @@ -140,6 +140,7 @@ mymain(void) DO_TEST("bhyveload-explicitargs"); DO_TEST("custom-loader"); DO_TEST("disk-cdrom-grub"); + DO_TEST("serial-grub"); virObjectUnref(driver.caps); virObjectUnref(driver.xmlopt); -- 1.9.3
participants (3)
-
Conrad Meyer -
Daniel P. Berrange -
Roman Bogorodskiy