Petr Viktorin wrote:
On 02/15/2013 08:18 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/15/2013 04:38 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
ipa-replica-conncheck ran SSH in quiet mode, probably to suppress a
message about connecting to an unknown host. This made it hard to
debug
connection errors.
I didn't find a way to separate SSH output from the output of the
called
command, I decided to try an additional SSH connection before calling
conncheck. SSH is set to verbose and if it fails the errors get
printed
out. Also, the host is added to a temporary known_hosts file.
The second SSH is called without the -q flag so errors/warnings are
not
lost even if the command fails. The temporary known_hosts file is used
so the unknown host warning doesn't appear here.
https://fedorahosted.org/freeipa/ticket/3402
The general procedure looks good, I don't think we should hardcode the
path to ssh. ipautil.run() overrides the current environment so we
should be able to safely run just 'ssh'.
We eventually need a cross-platform way of locating system binaries.
The hardcoded path to ipa-replica-conncheck is probably ok since we
provide that binary ourselves.
rob
Changed, thanks.
Looks and works well. I just have one final question. Should remote_addr
and temp_known_hosts be passed in as args? They are basically globals
but it is obvious where they came from, so not really a NAK, just a
question.
rob
Well, it's a style issue so I only have a bunch of rule-of-thumb
handwaving to justify the decision... Here goes:
Closing over the variables in main() doesn't have drawbacks of globals:
nothing outside main() can see/modify them and it doesn't keep main()
from (indirectly) calling itself (this particular main() probably won't
need to do that, but in general it's good to avoid this kind of surprises).
Passing the variables in as arguments would only make sense if run_ssh
was a top-level function, otherwise you have to worry about the right
order/names of arguments and gain nothing.
The nested function makes it clear that this is just a quick way to keep
things DRY, not a full-featured generic SSH caller. A top-level
"run_ssh" helper shouldn't unconditionally disable StrictHostKeychecking
for example. The code is tied to the caller functionally, so I kept it
tied syntactically as well.
On the other hand, nested functions are "magic" that you need a good
reason for. I think the above is just reason enough, but I don't feel
that strongly about it. Here's a version with top-level run_ssh, push it
if it seems better to you.
Works for me. I pushed the second patch to master and ipa-3-1. It is
clear enough where the variables come from. We can revisit in the future
if this gets messy.
rob
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
