Hi Dermot,
Here are my comments. Overall it looks good.
usr/src/lib/install_target/Makefile: OK
usr/src/pkg/manifests/system-library-install.mf: OK
General:
--------
Should "controller" be added to "__all__" in __init__.py for the install_target
package?
controller.py
-------------
247: "an default" -> "a default"
227: Nit: Since "no_initial_logical" dominates "no_initial_disk" perhaps
place no_initial_logical before it in the method prototype?
343, 344: Extraneous 4 spaces of indentation
388: Nit: add_disk() -> add_disks() since it expects "disks"?
409: "or a tuple of Disk objects" should be list instead of tuple of Disk
Objects
428: "disk" -> "disks" since it can process lists of disks.
Also the docstring is inconsistent regarding whether only a single Disk
instance or a list of Disk objects is handled.
458: To tie in with the above, I think you could simplify 458-472:
if disks is None:
disks = list()
for current_disk in current_disks:
for discovered_disk in discovered_disks:
if current_disk.name_matches(discovered_disk):
disks.append(discovered_disk)
elif not isinstance(disks, list):
disks = [disks]
469-470: If you're not keen on the above suggestion then you can eliminate the
for loop by using list.extend() instead of list.append():
for single_disk in disk:
disks.append(single_disk)
-->
disks.extend(disk)
539, 558, 1018: Can you cache the return of platform.processor() in the __init__
method?
560: Change slice_list = [] to slice_list = list() because Drew prefers it ;-)
561-564: "slice" is a python reserved word. Can you use "slc" instead, which is
what the rest of TI/TD uses?
558-608: It seems that there is unnecessary duplication of the slice handling
code between 560-577 and 589-608. I think this could be refactored
and shortened. The nested slice handling loop at 589-608 is not
actually
linked with the partition iteration in its enclosing loop (ie. the
partitions
and slices are identified indpendently). How about something like this:
# Process partitions first on X86
if self.arch == 'i386':
for parts in partitions:
if parts.is_solaris and disk.label == "VTOC":
# Mark partition as ACTIVE to be sure, and change
# action to create to ensure active flag is set.
# Create shouldn't change existing VTOC unless the
# sizes differ for any reason, which they shouldn't
parts.action = "create"
parts.bootid = Partition.ACTIVE
# Process slices on both X86 and SPARC
slices_list = list()
for slice in slices:
if slice.name != "2":
if slice.size >= self.minimum_target_size:
slice_list.append(slice)
break
if len(slice_list) == 0:
# No useable slices, Clear the slices and add a
# root slice
disk.delete_children(class_type=Slice)
new_slice = disk.add_slice("0", start, slice_size,
Size.sector_units)
new_slice.tag = V_ROOT
new_slice.in_vdev = in_vdev
new_slice.in_zpool = in_zpool
else:
# We have a useable slice already so nothing more to do
return
634: The error message could specific to say what's unsuitable about the disks
(ie. non are big enough)
753: Perhaps remove "etc" from method name since we already define its path
with VFSTAB_FILE? It seems like we are abstracting it but then embedding
it again in the method name.
781: self._minimum_target_size is never reset to None so this is only ever
calculated
once. However the initialize() method which sets parameters that this method
depends
on (self._image_size) can be re-called with a different image_size parameter
which
will not be noticed by this method, causing it to return incorrect values.
This could be eliminated if initialize() was combined into __init__ per Drew's
suggestion
791: Since this dependends on the minimum_target_size @property method it is
also
impacted by 781 above.
812: "apending" -> "appending"
918: "backup up" -> "backed up"
933-934, 943, 951-954: Should this be removed?
1019: I think there is an easier way to get the Partition by just referencing
slices[0].parent. It should be the encapuslating partition.
1027: new_slice.tag = V_ROOT - Should this be set on SPARC also?
1018-1041 It seems that you don't need to identify the partition as such,
just the enclosing partition or Disk object for X86 and Sparc respectively
because you just want to add a slice to it using the same method and args.
How about this simplification:
parent = slices[0].parent # will be either a Partition(X86) or Disk (Sparc)
new_slice = parent.add_slice("0", start, slice_size, Size.sector_units)
new_slice.tag = V_ROOT # If not arch specific
if self._vdev is not None:
new_slice.in_vdev = self._vdev.name
if self._zpool is not None:
new_slice.in_zpool = self._zpool.name
return True
Thanks,
Niall
--
This message posted from opensolaris.org
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
