The rest of my review for the osol_install/profile code. I reviewed all
files, if not noted, no comments for that file.
General: I think, for clarity, that the DiskInfo, PartitionInfo and
SliceInfo classes should be in separate python modules.
disk_info.py:
> 85 raise ValueError, "%s not a recognized suffix"
Looks like you need a substitution value in this string.
General: The class FileSize seems like it is mis-named. If it really
represents a disk, partition, slice or file size, then maybe a better
name would be 'StorageSize', or something. Seeing it used in the code as
FileSize makes me think that we are working with a file only.
> 248 def get_extended_partition(self):
> 249 '''Find and return the Extended partition on this disk.
> 250 An AttributeError is raised if this disk has no Extended
> partition
> 251
> 252 '''
> 253 for part in self.partitions:
> 254 if part.is_extended():
> 255 return part
> 256 raise AttributeError, "No Extended partitions on this disk"
General question... if you are raising an attribute error in this case,
what does the consumer do? It seems to me that there could be no
extended partitions, right? So, why don't you return a null value
instead? Same for get_solaris_data(). It could be that a device has no
solaris partitions or slices on it, correct?
> 309 elif self.slices:
> 310 use_partitions = False
> 311 parts = copy(self.slices)
> 312 parts.sort(cmp=SliceInfo.compare)
> 313 numbers = range(DiskInfo.MAX_SLICES)
> 314 numbers.remove(SliceInfo.BACKUP_SLICE)
> 315 start_pt = 0
First, the method name is add_unused_parts(), so why are we operating on
slices here? Method name and comments do not match functionality
provided in this method. I think it would be better to separate out the
partition functionality and slice functionality into two separate
methods. Same for add_unused_parts() method for PartitionInfo class.
> 331 if not use_partitions and part.number ==
> SliceInfo.BACKUP_SLICE:
> 332 continue
I don't think you need this check. you have removed the backup_part, if
not none, from the 'parts' array at line 325 so it shouldn't be
something you need to check again.
> 382 part_num = 5
Would be cleaner to define a value, such as START_LOGICAL_PARTITION = 5,
rather than hard code the 5 here.
> 429 logging.debug("Create tgt.Disk - geometry.blocksz = " +
> str(g.blocksz))
> 430 logging.debug("Create tgt.Disk - geometry.cylsz = " +
> str(g.cylsz))
> 431 logging.debug("Create tgt.Disk - name = " + name)
> 432 logging.debug("Create tgt.Disk - blocks = " + str(blocks))
> 433 logging.debug("Create tgt.Disk - controller = " +
> str(controller))
> 434 logging.debug("Create tgt.Disk - boot = " + str(boot))
> 435 logging.debug("Create tgt.Disk - removable = " + str(removable))
> 436 logging.debug("Create tgt.Disk - vendor = " + str(vendor))
> 437 logging.debug("Create tgt.Disk - serialno = " + str(serialno))
> 438 logging.debug("Create tgt.Disk - use_whole = " + str(use_whole))
> 439 logging.debug("Create tgt.Disk - fdisk = " + str(fdisk))
> 440 logging.debug("Create tgt.Disk - vtoc = " + str(vtoc))
> 441 logging.debug("Create tgt.Disk - child_list_size = " +
> 442 str(len(child_list)))
The debug logging would be better put in a separate utility python
module which you call.
> 788 '''Returns True if it is possible to edit this partition's size'''
> 789 if self.is_extended():
> 790 for logical in disk.get_logicals():
> 791 if logical.id != PartitionInfo.UNUSED:
> 792 return False
If a logical partition is not unused, isn't it still possible for users
to edit the size, which of course would potentially trash other data.
Not sure why it has to be unused to be modifiable.
install_profile.py:
> 27 including the disk target, netwrok configuration, system details (such as
nit: should be 'network'.
> 187 if dhcperr:
> 188 logging.warn("Error ocurred during dhcpinfo call: " +
> str(dhcperr))
If you return the dhcperr here, and the callers, such as find_dns():
> 99 dns_server = self._run_dhcpinfo("DNSserv")
> 100 if dns_server:
> 101 self.dns_address = dns_server
> 102 return True
Get a value from this, that isn't say a dhcp server, shouldn't you have
returned None, instead for this case in _run_dhcpinfo()?
> 47 self.is_role = is_role
With the liveCD GUI, you can only enter a user which is a role. No root
user is allowed now. Is this the case with the text installer? So,
shouldn't is_role always be True?
thanks,
sarah
****
