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
