#545: Use ssh ProxyCommand instead of multiple ssh commands --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: major | Milestone: Component: SimFactory | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
The quote handling in this patch is off. Simfactory has a function QuoteSafe that quotes its argument, so that it can safely be passed to other commands such as bash or ssh. Instead of removing quotes or adding "" around a string, QuoteSafe should be used.
The current trampoline code is written recursively and can handle multiple levels of trampolines. I am not aware of a current machine that requires this, but it was necessary for the SiCortex test machine. If we wanted e.g. to access LONI compute nodes we would still need this. The patch seems to remove the recursion and will thus handle only a single level of trampolines. This may be fine, but there then needs to be a check to ensure that the trampoline does not require a trampoline. Can the ProxyCommand mechanism be nested? Do we want this functionality? It would be elegant, but does complicate things.
The current code is a bit convoluted, because the trampoline handling code is called both for ssh and for rsync commands. You implement this via an explicit switch (test if the argument is RSYNC); this may be a good idea, but an additional boolean argument may be a cleaner approach. Also, the current patch doesn't make it clear under which circumstances cmd would be empty or non-empty. I guess this is for "login" and "execute", respectively?
The nc command should be quoted with QuoteSafe instead of an explicit backslash.