Virginia Wray wrote: > Hi Joe - > > comments in line..... > > Joseph J VLcek wrote: >> Virginia Wray wrote: >>> Could someone take a look? >>> CR 1092 cleans up some files and directories that are left on the >>> system after the live cd installation has completed. >>> >>> >>> CR: >>> http://defect.opensolaris.org/bz/show_bug.cgi?id=1092 >>> >>> Webrev: >>> http://cr.opensolaris.org/~ginnie/1092/ >>> >>> thx, >>> >> >> >> Issue 1 - Miner Nits: >> --------------------- >> >> Lines: >> >> 602 #Cleanup the files... >> 603 #the mp directory that ... >> >> and Lines >> >> 616 #The bootcd_microroot directory ... >> 617 #distroconstructor once they ... >> >> Nit 1: All other comments have a blank space after the # >> Nit 2: Shouldn't distroconstructor be two words or hyphened? > Thanks for pointing that out. I'll add the space and clean up the DC entry. > >> >> Issue 2 - Consider not raising an exception if os.rmdir fails... >> ------- >> >> 619 cleanup_dir_list = [ "/a", "/bootcd_microroot" ] >> 620 >> 621 for dname in cleanup_dir_list: >> 622 self.check_abort() >> 623 try: >> 624 os.rmdir(mp + "/" + dname) >> 625 except: >> 626 pass >> >> Will this code raise the exception if os.rmdir fails to remove one of >> the directories listed in cleanup_dir_list? >> >> The comment states that bootcd_microroot will be cleaned up by the DC >> in the future. >> >> If so it might be better to not have an exception raised by this code >> if the bootcd_microroot directory is not there. > Actually, what this exception does is ignore a failure and move on. So, > if bootcd_microroot isn't there and removal of the directory fails, the > control is passed to the exception , but because the exception is set to > pass, the failure is ignored. Make sense?
Doh! Yes that makes sense. I just read it backwards... Thanks! Looks good. Joe > > thanks for your feedback. > ginnie > > >> >> >> Hope this helps. >> >> Joe >> >> >
