[Libvir] [PATCH] add bootloader_args to libvirt xml to support bootloaders (like pypxeboot for example) that require arguments
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... --Hugh -- Red Hat Virtualization Group http://redhat.com/virtualization Hugh Brock | virt-manager http://virt-manager.org hbrock@redhat.com | virtualization library http://libvirt.org
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...
Looks good to me. Did you find out what was causing the build problems (ie. beyond just NFS)? Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
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 ? thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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@redhat.com | virtualization library http://libvirt.org
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (3)
-
Daniel Veillard -
Hugh Brock -
Richard W.M. Jones