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

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.