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

[Daniel Veillard]

On Tue, Jun 05, 2007 at 08:35:21AM -0400, Hugh Brock wrote:
> Daniel Veillard wrote:
> >  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.

  I remember the 'yuck but I don't see any other way' feeling when we
added this. But since 1/ I can't remember precisely 2/ you and Dan looked
at it and couldn't find either, I'm fine taking the very small risk of breaking
something in an older release. Libvirt is by essence a place where nasty
tricks like this will pile over time, and while we really try to keep
compatibility, if really we can find/remember why one could be needed
cleanup makes sense.
  Let's apply the patch then ! Will do shortly unless you disagree,

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/