Thanks for the review and comments, Karen and Keith.
Sue
On 02/14/11 01:59 PM, Karen Tung wrote:
Hi Sue,
Everything looks good to me now.
--Karen
On 02/14/11 13:31, Sue Sohn wrote:
Hi Karen,
After our offline chat with Keith, I've modified the code to return a 1 if the
script fails, rather than passing on the return code. I also addressed the nit
you mentioned in your email. webrev is updated.
Sue
On 02/14/11 12:17 PM, Keith Mitchell wrote:
Hi Karen,
Sue may answer more specifically, but from a general "philosophical" standpoint,
exceptions are for (generally unexpected) exceptional conditions. So if a
command is actually expected to have a non-zero return, then I think it may be
better to check the return code.
The best example may be to pretend we have a function to call "pkg install <some
pkg>", which returns 0 if it succeeds, 4 if the given pkg is already installed
on the system, and 1 for a failure to execute (e.g., permissions error). So one
might write a function "do_pkg_install":
def do_pkg_install(pkg):
cmd = ["pkg", "install", pkg]
pkg_popen = Popen(cmd, check_result=[0, 4])
if pkg_popen.returncode == 4:
print "Nothing to do"
else:
print pkg + " installed successfully!"
The expected (check_result) cases are handled gracefully, whereas unexpected
failures from permissions will lead to exceptions raised to the caller, who can
deal with them as needed.
Of course, the line between "expected failure" and "unexpected failure" is a
blurry one at times, so it's very context sensitive as to which approach to
take.
The other key point to make is that exceptions are used to *ensure* that an
error case that the local scope can't handle gets dealt with by the caller (or
its caller, or its caller, or...). If the error case is either handled (or
propagated/translated to a different exception) in the local scope, then there's
not as much benefit to the overhead of generating the exception.
- Keith
On 02/14/11 11:52 AM, Karen Tung wrote:
Hi Sue,
Question: In all the Popen calls in create_client.py and installadm.py, why not
utilize the check_result argument of the Popen() call?
I think that option in Popen() is provided to avoid these return code
checking.
Another nit in create_client.py and installadm.py is that the code have:
cmd = [CHECK_SETUP_SCRIPT]
ret = Popen(cmd).wait()
Why not just do:
ret = Popen([CHECK_SETUP_SCRIPT])
Thanks,
--Karen
On 02/14/11 11:25, Sue Sohn wrote:
Could I please get a review of the changes for:
7019438 Remove run_script from installadm
Webrev:
http://cr.opensolaris.org/~sohn/7019438
Thanks,
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
