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
