Niall,
Thanks for reviewing. Responses below.
On 05/04/11 10:40, Niall Power wrote:
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?
yes - done.
controller.py
-------------
247: "an default" -> "a default"
fixed
227: Nit: Since "no_initial_logical" dominates "no_initial_disk" perhaps
place no_initial_logical before it in the method prototype?
done
343, 344: Extraneous 4 spaces of indentation
I disagree. Each parameter's description is indented, if it runs
to more than one line. It just happens that only this one does,
so it might look a bit strange.
388: Nit: add_disk() -> add_disks() since it expects "disks"?
Well, it takes a disk or a list of disks, so it could be either.
As this is a well-established API at this point, (GUI, Text and AI
are already using it), I'd prefer to leave it as it is, if that's OK?
409: "or a tuple of Disk objects" should be list instead of tuple of Disk
Objects
fixed
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)
This is a mistake - the disk parameter should only be a single
disk, not a list of disks. The scenarios are too, too complicated
if there are multiple disks selected, and the user tries to reset
several, but not all, of them.
I didn't notice this change being made and am now reversing it.
I have removed the code that allows this param to be a list - it must
be None or a singleton Disk.
539, 558, 1018: Can you cache the return of platform.processor() in the __init__
method?
done
560: Change slice_list = [] to slice_list = list() because Drew prefers it ;-)
then it shall be so...
561-564: "slice" is a python reserved word. Can you use "slc" instead, which is
what the rest of TI/TD uses?
fixed
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
they are linked, in the sense that then are only being performed
when the Solaris2 partition on a VTOC disk has been identified,
and the slices are being modified on that Solaris2 partition...
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)
I disagree here - on i386, the slices are being modified on
the Solaris2 partition, not the Disk...
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
I agree, though, that this code could be refactored and have done
so, taking parts of your and Drew's suggestions.
634: The error message could specific to say what's unsuitable about the disks
(ie. non are big enough)
Done.
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.
OK - done (as this is a very new addition to the API, changing it won't have
much impact)
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.
Good catches - you must have actually read this code pretty well!
As per response to Drew, combining initialize() and __init__() is not desirable,
so I have reset self._minimum_target_size to None initialize() to force this
value to be recomputed any time _image_size can change.
812: "apending" -> "appending"
done
918: "backup up" -> "backed up"
done
933-934, 943, 951-954: Should this be removed?
done
1019: I think there is an easier way to get the Partition by just referencing
slices[0].parent. It should be the encapuslating partition.
agreed - done
1027: new_slice.tag = V_ROOT - Should this be set on SPARC also?
I don't know - I'm leaving this for Evan to answer.
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
agreed - done
Many thanks,
- Dermot
Thanks,
Niall
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
