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...

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 ?

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,