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

[Roman Bogorodskiy]

  Conrad Meyer wrote:

> On Sun, Oct 26, 2014 at 7:00 AM, Roman Bogorodskiy
> <bogorodskiy at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141026/6c7f73de/attachment-0001.sig>