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
