Hi Dermot,
Thanks for reviewing this. Comments below...
On 12/21/11 02:48 AM, Dermot McCluskey wrote:
Niall,
This is my review of the Targets section. GUI review will follow.
Looks good. Mostly minor nits, corrections to comments, etc:
General comment: your copyright notices will probably be "2012"
by the time you push.
**
*Targets: Dermot, Darren, Matt
---------------------------------------*
usr/src/pkg/manifests/system-library-install.mf
usr/src/lib/install_manifest/dtd/target.dtd
76 gpt_partition, mbr_partition and slice names are numeric values,
77 e.g. 1, will be interpreted as partition 1 or slice 1.
Suggest change to:
76 gpt_partition names are numeric values, e.g. 1 will be
77 interpreted as partition 1.
as mbr_partition isn't defined and slice names are referred
to in existing comment at 119.
Will do.
109 - are "esp" and "bbp" not relevant here?
No. A decision was reached that we do not require or even want users
specifying special partitions for a few reasons:
- it makes the manifests less portable because they may not be
universally bootable (eg. a bios boot partition is of no use to UEFI)
- it is non-optional backend plumbing that the user shouldn't have to
figure out nor be given the opportunity to break. Excluding them
keeps the manifest format simpler and more consistent and avoids
introduction of a new stream of manifest error conditions.
114: are curly braces really required when using GUIDs?
They are not required but the standard representation of GUIDs includes
curly braces. A hex string without the curly
braces will also work too if specified in the manifest.
usr/src/lib/install_target/Makefile
usr/src/lib/install_target/__init__.py
usr/src/lib/install_target/controller.py
usr/src/lib/install_target/cuuid.py
45 # We do make this one as compatible as possible with uuid.UUID
Wording seems strange? Not sure what's meant here.
We try to implement API level compatibility with the standard python
uuid.UUID class via the methods and properties
defined by cUUID. Much of the code in cUUID is from python uuid.UUID and
just tweaked to play well with C types.
Does this clarify, or would you prefer that comment to be modified?
usr/src/lib/install_target/discovery.py
usr/src/lib/install_target/instantiation.py
usr/src/lib/install_target/libdiskmgt/diskmgt.py
usr/src/lib/install_target/libefi/Makefile
usr/src/lib/install_target/libefi/__init__.py
usr/src/lib/install_target/libefi/cfunc.py
usr/src/lib/install_target/libefi/const.py
usr/src/lib/install_target/libefi/cstruct.py
66 # The number of GPE entries that would fit into 16K */
remove trailing "*/" ;)
Oops ;-)
usr/src/lib/install_target/libefi/efi.py
usr/src/lib/install_target/logical.py
266: what does zpool -B do? it's not documented in man page.
It is a new feature that we have requested from the ZFS team. It allows
creation of a bootable zpool on a whole disk. ZFS will take care of
partitioning the disk and creating the required EFI system or BIOS boot
partition as well as the Solaris reserved partition. Using this option
means we don't have to partition the disk up within install before
passing it to zpool.
usr/src/lib/install_target/physical.py
318 resized_gpart = self.parent.add_gptpartition(self.name,
319 start_sector, size, size_units, self.guid, self.in_zpool,
320 self.in_vdev)
keyword params should be specified using keywords,
and not treated as positional arguments, ie:
318 resized_gpart = self.parent.add_gptpartition(self.name,
319 start_sector, size, size_units=size_units,
partition_type=self.guid, in_zpool=self.in_zpool,
320 in_vdev=self.in_vdev)
Will do
1935 sys_part = self.add_gptpartition(str(index), gptsys_start,
1936 sys_size.sectors, Size.sector_units,
1937 partition_type=self.sysboot_guid, force=True)
and
2048 resv_part = self.add_gptpartition(str(index), resv_start,
2049 resv_sectors, Size.sector_units,
2050 partition_type=efi_const.EFI_RESERVED, force=True)
Similarly, specify "size_units=Size.sector_units".
Will do.
949 if self.part_type == 239:
I'd prefer to see something like:
949 if self.part_type == self.name_to_num("EFI System"):
for consistency with other code.
My reference for consistency was the existing property immediately
beneath :-)
def is_solaris(self):
""" is_solaris() - instance property to return a Boolean value
of True
if the partition is a Solaris partition
"""
if (self.part_type == 130 and not self.is_linux_swap) or \
self.part_type == 191:
return True
return False
We should change this one too if we proceed with this.
Also the existing implementation can determine a result between 239 to
255 times faster
than using name_to_num() for True and False results because it requires
no dictionary iteration.
If you still favour using name_to_num() I will change it however.
1395 return [gpart for gpart in gptpartitions]
why not just:
1395 return gptpartitions
as there is no "if" clause here?
Agreed - makes no sense whatsoever :-)
1849 return
should this not be:
1849 return None, None
Yes, it should. Will fix.
1965... can get_next_partition_name() not be used here?
No. get_next_partition_name() returns a balue based on availability
within the 7 user accessible partition names in the
range 0 - 6 inclusively.
The Solaris reserved should have a name/index of at least 8 or greater
so that it does not use one of the limited number
of user accesible partition names available. So
get_next_partition_name() would not be helpful here.
2208... can self.label not be used here?
No.
It seems wrong to use presence of Slice or Partition
children to determine VTOC/GPT
We had it implemented like that up until a week or so ago but there are
cases where disk.label can be inconsistent with
the children of the disk. If there are Slice or Partition children on
the disk then we know that there is geometry information
available for the disk (cylinders, sectors and heads). The presence of
disk.label = VTOC does not ensure this. The get_gaps
code for VTOC requires cylinder geometry information to calculate VTOC
gaps and will raise an exception if it isn't available.
So checking for Partition or Slice ensures this.
GPT only needs block size of the disk to calculate gaps, since it
doesn't care about cylinders heads and sectors so it is the
default method for this reason and preferece for GPT now.
- if disk has no children,
then it will be assumed to be GPT, right?
Yes.
usr/src/lib/install_target/shadow/__init__.py
124-126 remove commented out code
Done (by Dave)
usr/src/lib/install_target/shadow/physical.py
720-726: nice!
:-)
Thanks Dermot!
Cheers,
Niall
usr/src/lib/install_target/test/test_shadow_list.py
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
