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
