Re: [PATCH] nested partitions

[Vladimir 'phcoder' Serbinenko]

On Fri, Jul 31, 2009 at 9:58 AM, Marco Gerards<address@hidden> wrote:
> "Vladimir 'phcoder' Serbinenko" <address@hidden> writes:
>
>> Rediff
>>
>> On Thu, Jun 11, 2009 at 5:51 PM, Vladimir 'phcoder'
>> Serbinenko<address@hidden> wrote:
>>> Hello. Here is a first version of nested partition support. Beware
>>> it's EXPERIMENTAL and may be dagerous. Try at your own risk. With this
>>> patch you lose however the ability to specify bsd partition without
>>> specifying slice. Use search.
>
> Can you please explain what you mean by that you have to specify the
> slice and you have to use search?
Currently to access the BSD partition (hd0,1,a) user can type (hd0,a)
omitting '1' if he has no other BSD partitions. This may be convinient
but is somewhat ad-hoc and needs code in core to support this. With
using search I mean that if the number '1' isn't known user has to use
"search"
>
> What makes this experimental and dangerous?  Can you send in a patch
> that isn't?
Only that I touch core size and when I submitted it, it was only few
days old. Now I use it for over a month and haven't hit any problem
>
> Here a only a few comments on this patch.  In order to properly review
> it, can you explain what the design of the patch is and how it
> essentially works?  It is important to discuss that as well.  Can you
> also describe which problems the patch solves and introduces?
The typical case of the problem it solves is Solaris. When installed
on x86 its partition is subpartitioned. Because of that current grub
can't access its filesystem even if it's UFS or even when ZFS module
is loaded. sun_pc subpartitioning style will be a subject of separate
patch.
Something similar is used for BSD but it's done in an ad-hoc manner in
pc.mod which takes core size uselessly when user has e.g. /boot on lvm
on pc-style. With this patch bsdlabel.mod is a separate module
This patch mainly modifies the translation from sector relative to
partition start to the sector relative to start of disk. Inside every
partition it tries to find a possible subpartition.
Name parsing which was previously a part of every module has been
unified and consolidated in the core.

Could you perhaps quote only parts of patch which you comment
otherwise I have to scroll a lot. Thanks
> Please start with a capital letter and end with a `.'.  I noticed that
> you have committed other patches with this problem in the changelog
> entry.  If you commit something else and if you have the time, I would
> be happy if you could fix that too :-)
Already changed. This patch was sitting for too long
>> +     * partmap/acorn.c (acorn_partition_map_iterate): don't force raw access
>> +     Use number instead of index
>
> What do you mean by "don't force raw access use number instead of
> index"?
It was 2 sentences. Sorry for missing comma and quotes around field
names ('number' and 'index'). Actually I saw that eliminating 'index'
field isn't such a good idea after all. I'll make a patch which
doesn't eliminate it. If we keep 'index' we can remove 'data' field
which causes unstraightforward handling of allocated memory. Keeping
'index' and eliminating 'data' decreases core size for 50 bytes. The
only cost is that not size critical modules may need to re-read
parition info using 'offset' and 'index'
>> -  if (file->device->disk->partition)
>> -    part_start = grub_partition_get_start (file->device->disk->partition);
>> +  part_start = 0;
>> +  for (part = file->device->disk->partition; part; part = part->parent)
>> +    part_start += grub_partition_get_start (part);
>
> Why do you need this change?  Can you explain what this does?

It's because grub_partition_get_start resolves only one level of
partitioning and there may be more than one.

>> diff --git a/include/grub/bsdlabel.h b/include/grub/bsdlabel.h
>> new file mode 100644
>> index 0000000..ba75897
>> --- /dev/null
>> +++ b/include/grub/bsdlabel.h
>> @@ -0,0 +1,91 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 1999,2000,2001,2002,2004,2007  Free Software Foundation, 
>> Inc.
>
> 2009?

bsdlabel.h is mainly a copy paste from pc_partition.h and I didn't
feel that copyright claim on this move would be justified

>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git