[libvirt] [PATCH 3/4] xen: fix PyGrub boot device order

[Philipp Hahn]

When PyGrub is used as the bootloader in Xen, it gets passed the first
bootable disk. Xend supports a bootable-flag for this, which isn't
explicitly supported by libvirt.
When converting libvirt-xml to xen-sxpr the bootable-flag gets
implicitly set by xen.xend.XenConfig.device_add() for the first disk
(marked as Compat hack -- mark first disk bootable).
When converting back xen-sxpr to libvirt-xml, the disks are returned in
the internal order used by Xend ignoring the bootable-flag, which
looses the original order. When the domain is then re-defined, the order
of disks is changed, which breaks PyGrub, since a different disk gets
passed.

When converting xen-sxpt to libvirt-xml, use the bootable-flag to
determine the first disk.

This isn't perfect, since several disks can be marked as bootable using
the Xend-API, but that is not supported by libvirt. In all known cases
relevant to libvirt exactly one disk is marked as bootable.

Signed-off-by: Philipp Hahn h...@univention.de
---
valgrind might complain about an uninitialized read access when copying
disks[0] to disks[0], but that is overwritten again with the following
assignment.

https://forge.univention.org/bugzilla/show_bug.cgi?id=21938

 src/xenxs/xen_sxpr.c |   12 +++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c
index 72322a7..038c6bb 100644
--- a/src/xenxs/xen_sxpr.c
+++ b/src/xenxs/xen_sxpr.c
@@ -342,20 +342,24 @@ xenParseSxprDisks(virDomainDefPtr def,
 const char *src = NULL;
 const char *dst = NULL;
 const char *mode = NULL;
+const char *bootable = NULL;
 
 /* Again dealing with (vbd...) vs (tap ...) differences */
 if (sexpr_lookup(node, device/vbd)) {
 src = sexpr_node(node, device/vbd/uname);
 dst = sexpr_node(node, device/vbd/dev);
 mode = sexpr_node(node, device/vbd/mode);
+bootable = sexpr_node(node, device/vbd/bootable);
 } else if (sexpr_lookup(node, device/tap2)) {
 src = sexpr_node(node, device/tap2/uname);
 dst = sexpr_node(node, device/tap2/dev);
 mode = sexpr_node(node, device/tap2/mode);
+bootable = sexpr_node(node, device/tap2/bootable);
 } else {
 src = sexpr_node(node, device/tap/uname);
 dst = sexpr_node(node, device/tap/dev);
 mode = sexpr_node(node, device/tap/mode);
+bootable = sexpr_node(node, device/tap/bootable);
 }
 
 if (VIR_ALLOC(disk)  0)
@@ -481,7 +485,13 @@ xenParseSxprDisks(virDomainDefPtr def,
 if (VIR_REALLOC_N(def-disks, def-ndisks+1)  0)
 goto no_memory;
 
-def-disks[def-ndisks++] = disk;
+/* re-order disks if there is a bootable device */
+if (STREQ_NULLABLE(bootable, 1)) {
+def-disks[def-ndisks++] = def-disks[0];
+def-disks[0] = disk;
+} else {
+def-disks[def-ndisks++] = disk;
+}
 disk = NULL;
 }
 }
-- 
1.7.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/4] xen: fix PyGrub boot device order

[Eric Blake]

On 10/12/2011 02:26 AM, Philipp Hahn wrote:

When PyGrub is used as the bootloader in Xen, it gets passed the first
bootable disk. Xend supports a bootable-flag for this, which isn't
explicitly supported by libvirt.


Hmm, the XML has been enhanced in the meantime; we can now mark various 
disks as bootable (and in fact, in which order they should be attempted 
at boot) when targeting qemu.  Why can't the same xml changes be made to 
affect xen?  But that can be done as a separate patch.



When converting libvirt-xml to xen-sxpr the bootable-flag gets
implicitly set by xen.xend.XenConfig.device_add() for the first disk
(marked as Compat hack -- mark first disk bootable).
When converting back xen-sxpr to libvirt-xml, the disks are returned in
the internal order used by Xend ignoring the bootable-flag, which
looses the original order. When the domain is then re-defined, the order


s/looses/loses/


of disks is changed, which breaks PyGrub, since a different disk gets
passed.

When converting xen-sxpt to libvirt-xml, use the bootable-flag to


s/sxpt/sxpr/


determine the first disk.

This isn't perfect, since several disks can be marked as bootable using
the Xend-API, but that is not supported by libvirt. In all known cases
relevant to libvirt exactly one disk is marked as bootable.

Signed-off-by: Philipp Hahnh...@univention.de
---
valgrind might complain about an uninitialized read access when copying
disks[0] to disks[0], but that is overwritten again with the following
assignment.


No problem from valgrind; VIR_ALLOC_N guarantees that disks starts life 
initialized at 0 (that is, calloc, not malloc).



@@ -481,7 +485,13 @@ xenParseSxprDisks(virDomainDefPtr def,
  if (VIR_REALLOC_N(def-disks, def-ndisks+1)  0)
  goto no_memory;

-def-disks[def-ndisks++] = disk;
+/* re-order disks if there is a bootable device */
+if (STREQ_NULLABLE(bootable, 1)) {
+def-disks[def-ndisks++] = def-disks[0];
+def-disks[0] = disk;
+} else {
+def-disks[def-ndisks++] = disk;
+}


Feels a bit hacky, compared to a memmove to slide all disks; but if, as 
you say, xend already returns disks in an arbitrary order, then it 
doesn't matter if we preserve order.


ACK and pushed.

--
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list