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). > >> 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. 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. > >> 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. > > 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. > >> 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. > > > thanks, > sarah > **** > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
