Niall,
Responses below.
On 12/22/11 10:55, Niall Power wrote:
Hi Dermot,
On 12/21/11 10:54 PM, Dermot McCluskey wrote:
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;)
I'll do that. This is actually a module that Dave wrote and everyone
has their own commenting style,
but I agree that your suggestion is more explanatory :-)
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.
You are correct. I'm also not a 100% happy about the fact that label
and the children on the disk can be inconsistent.
I'm going to try to see what can be done to eliminate that. One
problem is that it's very easy for a client to set disk.label
without considering the impact on the disk (eg. inconsistencies).
Target controller is also partly responsible.
Agreed. And any improvements/simplifications you can make
there would be appreciated...
- 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?
Yes. There is one edge case and that is when a call is made to
get_gaps() if there are no
partition, slice or GPT partition objects on the disk but it started
out as a VTOC labeled disk
and had all its children removed. In such cases the get gaps call will
return a gap that is
slightly smaller (in the order of a few dozen kilobytes) than would
have been returned if it
was treated as an empty VTOC disk. I don't think the discrepency will
have an real world impact -
it would be a rare circumstance where a gap whose size match the exact
amount of space on
the whole disk was required. And even at that, if the client tries to
add a slice or partition, the shadow
code is going perform cylinder boundary adjustments on it so they will
frequently not get exactly what
they asked for.
If I can make some improvements to target controller I might be able
to avoid this type of second
guessing in get_gaps(). Target Controller is a tricky beast to tame
though :)
OK. I really just wanted to make sure you are aware of/have tested
for this scenario. As I've learned, partitions and/or slices never seem
to neatly fill up the entire disk - there are always some unused gaps
caused by their peculiarities. So, a small additional gap in an edge
case doesn't seem disastrous. And if you can refine this, then all
the better.
Also, looking again at _get_gpt_gaps(), I see that while it
works,
It works better - almost half the code of get_vtoc_gaps() :-P
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?
Good catch, I believe so. I can't see how it will ever be able to
process the final element in the usage list given the loop
incrementation and break out parameters. I'll test it out and remove
the last element if it works, which I expect it will.
Assuming that the backup GPT table is *always* at the very
end of the disk (which your code implies)
It is most definitely :)
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?
Yep. That makes sense. Will add assuming it works.
Great.
Thanks,
- Dermot
Thanks again!
Niall
- 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
