On Sun, Oct 26, 2014 at 7:00 AM, Roman Bogorodskiy <bogorodskiy@gmail.com> wrote:
Conrad Meyer wrote:
I've tested the HDD boot and it seems to work.
I've tried to boot from CD and had a problem with that. It generates a command like that:
/usr/local/sbin/grub-bhyve -v --root cd --device-map /usr/local/var/run/libvirt/bhyve/grub-bhyve-device.map_63694 --memory 512 linux3
and the error is:
Could not create VM linux3 Error in initializing VM
Map file looks this way:
(hd0) /home/novel/linux.img (cd) /home/novel/ubuntu-14.04-server-amd64.iso
Any idea what could be wrong with that?
It looks like the map files I've seen from GRUB documentation (and I've also tried cd0 and couldn't get grub-bhyve to boot that), but that might be legacy GRUB only. Maybe just (hd1) and root=(hd1) will work. I don't know.
@@ -157,5 +156,26 @@ An example of domain XML device entry for that will look like:</p> <p>Please refer to the <a href="storage.html">Storage documentation</a> for more details on storage management.</p>
+<h3><a name="grubbhyve">Using grub2-bhyve or Alternative Bootloaders</a></h3> + +<p>It's possible to boot non-FreeBSD guests by specifying an explicit +bootloader, e.g. <code>grub-bhyve(1)</code>. Arguments to the bootloader may be +specified as well. If no arguments are given and bootloader is +<code>grub-bhyve</code>, libvirt will try and boot from the first partition of +the disk image.</p> + +<pre> + ... + <bootloader>/usr/local/sbin/grub-bhyve</bootloader> + <bootloader_args>...</bootloader_args> + ... +</pre> +
I think it would be also great to add a complete domain XML for Linux guest into 'Example guest domain XML configurations' section.
Okay, will do.
+ if (def->os.bootloaderArgs == NULL) { + VIR_DEBUG("%s: bhyveload with default arguments", __func__);
Adding __func__ here is not needed because it'll be included in the log message as well, e.g.:
2014-10-26 10:16:28.285+0000: 34498181120: info : virBhyveProcessBuildGrubbhyveCmd:410 : virBhyveProcessBuildGrubbhyveCmd: Picking /home/novel/linux.img as HDD
(Yes, it's a different log entry, but idea is the same).
Sure, thanks. I will fix up this and the other __func__ / style / period issues.
+ /* XXX: Handle quoted? */ + blargs = virStringSplit(def->os.bootloaderArgs, " ", 0); + virCommandAddArgSet(cmd, (const char * const *)blargs); + virStringFreeList(blargs);
As this block is used two times without changes, I'm wondering if it's worth to move it out into a function as well or is it small enough for that...
I was on the fence. I think the risk of someone coming along and accidentally implementing quoting for only one of the sections is fairly low.
+ virDomainDiskGetSource(cd)); + } + + if (disk == NULL && + def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + disk = def->disks[i]; + VIR_INFO("%s: Picking %s as HDD", __func__, + virDomainDiskGetSource(disk));
Ditto wrt __func__.
Wondering if it would make sense to break the loop if cd != NULL and disk != NULL after the iteration as we pick the first match anyway?
Because of performance concerns, or for clarity? The number of disks you might typically have in a VM is so small, I think simpler code is maybe nicer. But if it's unclear, it can be changed.
+ + /* + * XXX Default grub-bhyve has some minor caveats, but MAY work for some + * typical configurations. In particular: + * + * - For HDD boot, assumes a GRUB install on hd0,msdos1 + */ + + VIR_FREE(privconn->grub_devicesmap_file); + rc = virAsprintf(&privconn->grub_devicesmap_file, + "%s/grub-bhyve-device.map_%ju", BHYVE_STATE_DIR, + (uintmax_t)getpid());
Could you please elaborate on deciding to store this filename in privconn? In this case we need to make sure that it's not a subject for races. Probably other option would be to make it per-domain based, as it's a per-domain resource anyway?
Domain-based would be better; privconn was just a guess based on 'pidfile'. Do we have some per-domain private data yet? And if so, where is it? Thanks for taking a look. Best, Conrad