Hi Keith,
A few comments inline... > Hi Sarah, > > Thank you for the review. I'll answer your comments on the profile code > here (including the comments from your other response). > > - Keith > >> disk_info.c: >> >>> 416 g = tgt.Geometry(blocksz=self.blocksz, cylsz=self.cylsz) >> >> maybe the variable name could be 'geo' or something more descriptive? > > The names here don't meet our pylint standards and will be updated and > made more descriptive. > >> >> -The classes: >> FileSize >> PartitionInfo >> SliceInfo >> >> Don't appear to be nested in the DiskInfo class. Just wondering why >> you have all of these in the same file? > > Originally the classes were small, and since they were so related it > made sense to put them in the same file. They've since outgrown this and > will be separated. FileSize, in particular, is due for a rename, and > will be moved to a common location in install for other consumers. > >> >> -I assume when you don't use the property() method in this file for >> getters/setters it is because they are more complicated setter/getter >> methods? For example, the get_extended_partition() method. Just >> wondering. > > I considered making it a property, but complex 'getter' functions work > less well as properties. Attribute access should be a fast operation - > I've seen recommendations that property getter/setters should not even > contain 'if' statements. While I don't agree with that degree of > strictness, these getters seem complex enough that leaving them as > functions seems wise. > > Since we're writing from scratch, there's no existing interface of > attribute access for those properties that I need to maintain, so going > with functions seems the best choice. > >> >> -You are using 1024 for KB in the text installer. Is the GUI the same? >> It didn't used to be. > > The GUI is the same. (See > http://opensolaris.org/jive/message.jspa?messageID=441787#441787) > >>> 327 min_gap_size = 1024**3 # 1 GB >> >> Couldn't you define the size, as you did in the FileSize class and not >> hard code it here? > > I can and should define this as a constant FileSize object. > > > Sarah Jelinek wrote: >> 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. > > Yes I do. > >> >> 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. > > I agree. I couldn't think of a better name at the time, so I went with > "FileSize," but StorageSize or DiskSpace or something similar would be > more appropriate (and I'm open to other suggestions if anyone happens to > think of any). > >> >>> 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? > > Correct. I originally thought it made more sense to fire off an error if > the consumer tried to get data that wasn't there, but I think returning > None instead will be more stable (especially when looking at how these > functions are utilized). In these cases, it seems that getting data that isn't there isn't an error. None makes more sense to me as a return value. > >> >>> 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. > > This, and similar scenarios, are used for interface compatibility. If > the same function signature is used, then the UI can call > "something.add_unused_parts()" without knowing if 'something' is a > DiskInfo or a PartitionInfo. This is useful because it significantly > reduces how often parts of the UI need to track whether we're > manipulating disk, partition, or slice data. > > That said, if 'add_unused_parts()' implies the wrong functionality, I'm > open to changing it to 'add_unused()' or 'add_unused_children()' or > something similar (same comment for other areas where the distinction > between partitions and slices becomes unclear. I think add_unused() would be fine. > > One piece I want to add is a formal declaration of the functions that > are used by both disks and partitions in the form of a common parent > class (perhaps an AbstractBaseClass) - same goes for the common > functions used by partitions and slices. >> >>> 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. > > Good catch. > >> >>> 382 part_num = 5 >> >> Would be cleaner to define a value, such as START_LOGICAL_PARTITION = >> 5, rather than hard code the 5 here. > > Yes. I'll add a constant for this. > >>> 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. > > I'll check with Karen, but I think these logging statements are mostly > temporary. I think that having debug logging, or debug code in from the start is fine, I just think it should be based on something the user sets to trigger the debug action. >> >>> 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. > > The UI spec calls for the Extended partition's size to be editable if > and only if there are no logical partitions contained within it. Why is this? So, if I was a user and I wanted to change the extended partition size as part of my install, what would I do? >> >> install_profile.py: >>> 27 including the disk target, netwrok configuration, system details >>> (such as >> >> nit: should be 'network'. > > Thanks. > >> >>> 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()? > > dhcperr isn't returned, it's logged. dhcpout is returned - if the error > was such that dhcpout is empty, line 100 will catch that. If in some > case there is error output, but also some standard output, then we > should try to use the data we got back. > Ok, I see that now. thanks for the clarification. >> >>> 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? > > The Text Installer maintains the GUI's original behavior; that is, root > is created as a normal user if no primary user is defined, and created > as a role otherwise. Why are we doing this? We modified the GUI to only allow a user, no root account, so why is the text installer allowing this? thanks, sarah
