This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] pygrub fails to read single partition with grub bootsect

To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Subject: Re: [Xen-devel] pygrub fails to read single partition with grub bootsector in 4.1.0-rc
From: M A Young <m.a.young@xxxxxxxxxxxx>
Date: Mon, 31 Jan 2011 11:59:43 +0000 (GMT)
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Delivery-date: Mon, 31 Jan 2011 04:01:31 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1296473059.4037.49.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <alpine.LFD.2.02.1101301935090.5024@xxxxxxxxxxxxxxx> <1296473059.4037.49.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (GSO 1167 2008-08-23)
On Mon, 31 Jan 2011, Ian Campbell wrote:

Thanks Michael.

On Sun, 2011-01-30 at 19:57 +0000, M A Young wrote:
I have a single partition (actually under lvm) which starts with a grub
boot sector.

Do you mean an unpartitioned disk (but with an MBR) as opposed to a disk
with a single partition?

Yes. It is what you get if have grub installed on a partition rather than in an MBR, and then tell xen to map that partition to a partition in the guest.

 pygrub in 4.0.1 coped with this successfully, but under
4.1.0-rc2 I get the error
Traceback (most recent call last):
   File "pygrub.orig", line 773, in <module>
     if not fs:
NameError: name 'fs' is not defined

The problem is when you install grub on a partition (it can be on the MBR
or on the boot sector of a partition), it installs an MBR-like boot
sector, in particular ending with 0xaa55 in bytes 510 and 511.

In 4.1.0 pygrub sees this and decides in get_partition_offsets() that it
is looking at an MBR, however when it checks the offsets it finds they are
all zero so returns an empty list of offsets to try, resulting in the
error above (in 4.0.1 the default is to return the offset in the first
partition, which is 0 so it worked). The attached patch aims to detect
this situation an return an offset of zero in this case, though perhaps it
makes sense to default to an offset of 0 rather than a blank list if no
appropriate offsets are detected.

Hrm, it's not clear to me that this situation is a valid one per what a
native BIOS would find acceptable.

AIUI (although I'm no expert here) the valid choices for disk contents
are either a completely raw disk (i.e. no MBR at all) or a disk with an
MBR and a partition table (possibly only containing a single large
partition) but not something in between as you have here. Strictly
speaking I suspect this is a bug in whatever created the MBR though.

I don't know whether it is actually needed for a bootable partition or not, but it does seem to be common practice to stick aa55 at the end of the first 512 byte sector of a bootable partition (I have just checked a Windows XP partition and it does the same though with text in what would be the partition table so 4.0.1 pygrub probably does something weird with it).

That said I'm inclined to think that we should be reasonably tolerant in
what we accept as input and that there is little harm in trying to treat
things as a whole disk image if we would have otherwise failed anyway
(worst case is we simply fail a bit later on). pygrub is read-only so
its probably not going to eat anything.

So I think it is acceptable that when get_partition_offsets() detects
this specific situation it behaves as if the is_disk_image test had
succeeded, i.e. returns [0] (which I presume is what part_offs.append(0)
you add effectively means since part_offs ==[] at this point). So:

Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

However we need a signed-off-by from you too before we can consider
applying the patch.

The patch in my previous email is
Signed-off-by: Michael Young <m.a.young@xxxxxxxxxxxx>

        Michael Young

Xen-devel mailing list