Hi John, Thanks for updating. One last question: did you revert the function name to "findTFTProot()"? Or did the latest webrev not pick up the name change? (The function name, for PEP 8 compliance, should be either "find_TFTP_root()" or "find_tftp_root()" - the capitals are acceptable b/c it's an acronym, but the underscores are 'required'). No need for a new webrev if the name gets changed again.
Everything else looks great! - Keith John Fischer wrote: > Keith, > > Thanks again. Still learning stuff about Python from you. > > I've implemented the suggestions and updated the webrev: > > http://cr.opensolaris.org/~johnfisc/13123-tftpboot > > Thanks, > > John > > Keith Mitchell wrote: >> Hi John, >> >> I have some additional comments based on the updated webrev and now >> that I have a better understanding of what this code is doing. >> >> 898: Nit: shell defaults to False, so it's not strictly necessary to >> pass in this parameter, though it doesn't hurt. >> >> 901: For readability, can we call this variable something like >> "output"? Or, even better, assign to two separate variables, such as: >> "stdout, stderr = pipe.communicate()" >> >> 909: To simplify this line, and conform to general PEP 8 guidelines, >> this can be replaced with "if tup[1]:" (or "if stderr:" if following >> my above recommended name change) >> >> 916-924: I think this could be simplified to be more 'self-reading': >> >> svcprop_out = stdout.partition(" -s ") # This returns a tuple of >> length 3, splitting the output around the " -s " >> basedir = svcprop_out[2].rstrip("\n") >> if not basedir: # In case the result was empty >> basedir = defaultbasedir >> >> (Additional advantage of using the 'partition' function - even if >> stdout doesn't have a " -s " anywhere in it (unlike 'split' which >> returns a variable length tuple. However, if there's worry about the >> " -s " appearing earlier, for example in the tftpd command somehow, >> 'split' could be used, and then the length of the resulting tuple >> examined before taking the last item in the tuple as the basedir.) >> >> Thanks! >> - Keith >> >> >> John Fischer wrote: >>> Keith, >>> >>> Thanks for the review. >>> >>> I missed the comment on Pipe.wait(): >>> >>> This will deadlock if the child process generates enough >>> output to a stdout or stderr pipe such that it blocks waiting >>> for the OS pipe buffer to accept more data. Use communicate() >>> to avoid that. >>> >>> The output from the svcprop command is either (stdout): >>> >>> /usr/sbin/in.tftpd -s /tftpboot >>> >>> Or (stderr): >>> >>> svcprop: Pattern 'tftp/udp6' doesn't match any entities >>> >>> Or (stderr); >>> >>> svcprop: Couldn't find property group 'inetd_start/exec' for >>> instance 'tftp/udp6'. >>> >>> communicate returns a tuple of (stdout, stderr) where as >>> the read returns a string or -1. The values within the tuple >>> are both strings. The strings (for communicate and read) are >>> identical so the processing would be/is similar. I'll use >>> communicate. >>> >>> I have updated the webrev. >>> >>> http://cr.opensolaris.org/~johnfisc/13123-tftpboot >>> >>> Thanks, >>> >>> John >>> >>> >>> >>> On 01/26/10 11:57 AM, Keith Mitchell wrote: >>>> 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. >>> >>> OK. Makes for less changes. ;-) >>> >>>> 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. >>> >>> I will use communicate instead. >>> >>>> 888: stderr isn't redirected, which means the file descriptor will be >>>> inherited from this process. Is that intended? >>> >>> Will redirect the stderr output so that we can echo out the error with >>> something more understandable. >>> >>>> 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) >>> >>> Will comment. >>> >>>> 867-904: There doesn't appear to be any exception handling here - what >>>> happens if the Popen(...) or wait() calls fail? >>> >>> Will wrap around popen and communicate. >>> >>>> 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
