On Tue, Jun 05, 2007 at 08:55:34AM -0400, Daniel Veillard wrote: > 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, okay, commited, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/