Xen devel - Jan 2011 - pygrub fails to read single partition with grub bootsector in 4.1.0-rc

I have a single partition (actually under lvm) which starts with a grub 
boot sector. 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.

 	Micahel Young

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

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@citrix.com>

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

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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@citrix.com>
>
> 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@durham.ac.uk>

 	Michael Young

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2011-01-31 at 11:59 +0000, M A Young wrote: 
> 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.
oh, so you are using the "split disk" scheme where xvda1, xvda2 etc
are
configured for the guest separately, as opposed to the "whole disk"
scheme where xvda (possibly including a partition table) is configured,
but you are also doing "grub-install /dev/xvda1" or something similar.

In this scheme I don''t really see the benefit of installing grub into
the partition in this way -- nothing will ever be able to make use of
it. Note that installing grub to the partition is not a prerequisite for
having a valid grub configuration under /boot, the latter being all
pygrub requires.

I generally recommend people use the whole disk configuration anyway,
and I would redouble that in cases where pygrub is in use. About the
only time I would recommend the split disk configuration is where you
have the guest kernels etc stored in domain 0''s filesystem.

I think that the correct solution would be for pygrub to be passed an
option by the toolstack which indicates that it is being asked to
operate explicitly on a split disk partition and not a whole disk, and
therefore skip all the partition detecting stuff. I don''t know if
pygrub
can infer this from the existing options it gets and I suspect that
adding it would be a rather intrusive patch. Your patch seems like a
viable workaround.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel