Hi John, I have a few comments about the code. Looks like everything does what's needed, but I have some suggestions and thoughts for making the final function more legible.
installadm_common.py: 867: Nit: Despite Pylint's complaints to the contrary, it is acceptable (and expected) by PEP 8 standards to capitalize acronyms (in this case, TFTP) for readability purposes. It's completely up to you if you want to change it, however. 888: For commands with low amounts of output (i.e., output which would all fit within memory for all reasonable scenarios), use of the 'communicate()' function of the subprocess.Popen object generally leads to cleaner, simpler and more legible code. I can provide an example if you'd like. 888: stderr isn't redirected, which means the file descriptor will be inherited from this process. Is that intended? 892-901: What exactly is this code doing? That is, what's the expected output from svcprop that is being parsed? I suspect there's a better way to parse this output in Python that lends itself to more legibility (and if not, comments would help) 867-904: There doesn't appear to be any exception handling here - what happens if the Popen(...) or wait() calls fail? Thanks, Keith John Fischer wrote: > All, > > Can I get a couple of folks to look at the webrev: > > http://cr.opensolaris.org/~johnfisc/13123-tftpboot > > for: > > 13123 installadm needs to reference SMF for tftp's > base directory, not inetd.conf > > http://defect.opensolaris.org/bz/show_bug.cgi?id=13123 > > The fix is to use /usr/bin/svcprop to get the entry from SMF > via a pipe. If there is a problem then it falls back to /tftpboot. > > I've tested it with installadm list and installadm delete-service > which are the consumers of the interface. > > Thanks, > > John > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
