[Libvir] [PATCH] add bootloader_args to libvirt xml to support bootloaders (like pypxeboot for example) that require arguments

Hugh Brock hbrock at redhat.com
Tue Jun 5 12:35:21 UTC 2007


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 at redhat.com    | virtualization library http://libvirt.org




More information about the libvir-list mailing list