Keith, Ah, misunderstood what you were talking about. Yes. I will use find_TFTP_root(). I will also re-read PEP 8 now that I understand Python a little more.
John Keith Mitchell wrote: > 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
