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.
109 - are "esp" and "bbp" not relevant here?
114: are curly braces really required when using GUIDs?
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.
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 "*/" ;)
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.
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)
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".
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.
1395 return [gpart for gpart in gptpartitions]
why not just:
1395 return gptpartitions
as there is no "if" clause here?
1849 return
should this not be:
1849 return None, None
1965... can get_next_partition_name() not be used here?
2208... can self.label not be used here?
It seems wrong to use presence of Slice or Partition
children to determine VTOC/GPT - if disk has no children,
then it will be assumed to be GPT, right?
usr/src/lib/install_target/shadow/__init__.py
124-126 remove commented out code
usr/src/lib/install_target/shadow/physical.py
720-726: nice!
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
