webrev updated to match Dave's comments. http://cr.opensolaris.org/~johnfisc/list
Thanks, John John Fischer wrote: > Dave, > > Again, thanks!! Hopefully, with the comments the review > was easier. > > Comments below. > > Thanks, > > John > > > Dave Miner wrote: >> installadm.h. 54: nit, align expansion with neighboring lines > > Absolutely. This was an issue with my editor that changed the > indent for tab. > >> delete_service.py >> >> 187: the design for the INETd_CONF functionality is flawed (sigh), but >> that's not your fault; I've filed bug 13123 to cover this issue. > > Thanks. > >> 205, 218: stylistically, I'd think it makes more sense to put the >> comment inside the except block than outside. > > Moved the comment. > >> 222ff: This logic isn't quite the same as what in.tftpd uses when -s >> is specified (which is the normal and recommended case). If -s is >> used and the directory doesn't exist the the tftp service should go to >> maintenance. I think you can simplify on this basis. > > I am going to assume that this will be fixed when defect 13123 is resolved. > >> list.py >> >> 211: s/ot/to/ > > Corrected. > >> 246: seems that you should just use socket.gethostyname_ex() here > > That would be shorter (because I no longer would need an additional > function) and easier. > >> 311: don't grok this comment... > > Ok. > >> 316: should we be using getfqdn() instead? > > Yes. Will use it instead. > >> 322: this comment could be construed that you're creating a directory, >> might rephrase > > Ah, didn't mean that at all and I see how it could be construed that > way. Will reword. > > # get the Network IP path for the host > >> 379: this comment seems incorrect; /etc/netboot doesn't appear to be >> used anywhere here (correctly) > > Yeap. That's what I get for working late Friday night. I've updated > the comment appropriately. > > We first get the TFTProot directory. Then iterate over the > files within the TFTProot directory to get the client menu > which contains the Image Path and Service Name. The client > and image path are then added to the named service dictionary > list. > >> 560: how would this condition be the case? Should we do something >> stronger than just skip it? > > It shouldn't happen at all. If it does then there is something wrong > within the SMF service. I will send a message to stderr and exit. I > have code elsewhere as well and will treat the issue the same way. > >> 916: Seems like we can get some sort of traceback out of this >> operation, as I don't see except clauses that will catch a failure >> anywhere further up the chain. > > I missed that call to AIdb.DB and another set of AIdb calls. Will wrap > with try/except. > >> Dave > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
