Niall,
Responses below.
On 12/21/11 00:47, Niall Power wrote:
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.
ok - thanks for explanation.
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.
ok - i just they were accidentally included, but clearly this
is not the case.
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?
Something like "This code is intended to be as compatible
as possible with uuid.UUID." would read better for me. But
it's not a deal-breaker;)
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.
Good to know. Thanks for explanaing.
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.
You make a good point! Leave it as is.
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.
ok
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.
ok. perhaps add a brief comment then, eg: "we cannot
rely on self.label at this point for distinguishing between
GPT and VTOC"?
Also, comment at start of method:
""" get_gaps() - method to return a list of Holey Objects
(depending on architecture and disk label) available on the Disk.
"""
now seems wrong if disk label is not being used.
- if disk has no children,
then it will be assumed to be GPT, right?
Yes.
ok. are there any edge cases where this could cause problems?
eg if a non-GPT disk has no slices or partitions, it should
really report 1 gap consisting of the entire disk:
0L - self.disk_prop.dev_size.sectors
but if we end up calling _get_gpt_gaps() for an empty non-GPT disk,
it will report a gap consisting of the disk, minus the primary and
backup GPT tables?
Can this result in different behavior from the old code, ie slightly
different partition sizes?
Also, looking again at _get_gpt_gaps(), I see that while it
works, it doesn't really follow the same principal as
_get_vtoc_gaps() in relation to how the 'usage' list is
populated. ie you are putting an odd number of entries in
the list, and as they are processed in pairs, it should never
process the last entry? My guess is that if you delete line:
2077 usage.append(self.disk_prop.dev_size.sectors - 1)
it will work the same?
Assuming that the backup GPT table is *always* at the very
end of the disk (which your code implies) I think lines 2074 - 2077
should be something like:
2074 # The disk's end bookend is effectively the start of the
2075 # backup GPT table
2075 usage.append(self.disk_prop.dev_size.sectors - \
2076 self.gpt_backup_table_size.sectors)
Do you agree?
- Dermot
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
