9:19 a.m.
Conrad Meyer wrote:
On Sun, Oct 26, 2014 at 7:00 AM, Roman Bogorodskiy
<bogorodskiy(a)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.
My problem was that that Linux installer ISO shows GRUB interactive
menu. Looks like CDROM support is not very useful is this case :(
We need to teach virsh console to work for the loader also; that should
be easy for bhyveload, however, grub-bhyve doesn't seem to support that,
so it needs to be updated as well.
>> @@ -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?
Yes, we have that, there's bhyveDomainObjPrivate in bhyve_domain.h.
Thanks for taking a look.
Best,
Conrad
Roman Bogorodskiy