Re: [Libvir] [PATCH] add bootloader_args to libvirt xml to support bootloaders (like pypxeboot for example) that require arguments
Daniel Veillard wrote:
On Mon, Jun 04, 2007 at 05:08:14PM -0400, Hugh Brock wrote:
> This patch extends libvirt's XML to include a paramater Xen already
> supports in sexpr, "bootloader_args." The bootloader_args parameter lets
> you pass arguments -- "mac=aa:12:34:56:78:90" for example -- to a
> bootloader program that xend invokes when it starts a guest. One such
> program, pypxeboot, uses the mac= argument to communicate with a dhcp
> server and retrieve the pxeboot kernel and initrd for a paravirt guest.
>
> The patch includes new tests for the bootloader_args XML and sexpr,
> which pass.
>
> Note that for the head of current libvirt, the virsh tests do not pass
> and virsh will not connect to the xen hypervisor. I don't know if this
> is my own build environment or some other issue. However the failure is
> the same with or without this patch, so I am submitting it anyway.
>
> Please let me know what you think...
This all looks simple and clear except one point.
We used to process pygrub differently than other bootloaders, basically testing
for that value and making it a special case, ending up with 3 different case:
- no bootloader (bootloader == 0)
- bootloader is not pygrub (bootloader == 1)
- bootloader is pygrub (bootloader == 2)
I'm pretty sure we made that on purpose to avoid some problem, but I can't
remember why :-(. Your patch changes that and gets back to only 2 case, and
I don't see an explanation of what changed to drop the special case. Can
you explain why this was changed ? I also wonder what would happen if we drop
the new libvirt resulting from this say on a Fedora Core 6, would that break
pygrub in that environment due to the change ?
So this is an interesting question, and you may well be right that I
have removed some special case that was at some point (or is still)
important. Here is the full story as I understand it. In the old code,
both the no-bootloader case (bootloader=0) and the non-pygrub case
(bootloader=1) allow <kernel> and <initrd> in the xml along with the
<bootloader> element. pygrub takes no such arguments, so if that was the
bootloader being used (bootloader=2) the <kernel> and <initrd> were
omitted.
As you can imagine when I saw this code my first thought was "what the
hell is this" <g>, so I checked with Dan. We were both unable to imagine
a scenario in which having <kernel> and <initrd> in the xml along with
<bootloader> was useful, since if you know what kernel and initrd you
want to boot (for the paravirt case, of course), you don't need a
bootloader. We also didn't much like having a check for "pygrub"
hardcoded in libvirt, especially since I would have had to extend it to
be "pygrub" || "pypxeboot" (yuck).
If we really do need this special case then I'm more than happy to put
it (or some less nasty version of it) back. I agree that it does have
the look of something that was added for a specific edge case.
Take care,
--Hugh
--
Red Hat Virtualization Group http://redhat.com/virtualization
Hugh Brock | virt-manager http://virt-manager.org
hbrock(a)redhat.com | virtualization library http://libvirt.org