Draft version of patch for the resolving the issue1371. 
It does auto-detection of the plink/ssh implementation using -V option.
It provides unified data structure for introduction other ssh impl
specifics opts.

Its going to be amended soon, purpose of this patch is disclosure
choosen approach.

Attachments

• resolve-issue1371_-darcs-assumes-_p-for-ssh_port_-but-putty-takes-_p.dpatch

With Guillaume's accord, I'm assigning him to have a look at this draft
patch.  (It's good for Darcs to gain reviewers; it's good for as many
reviewers to see as much of the Darcs code as possible IMHO)

Guillaume: you may want to look at the open issues on the tracker that
have the SSH topic in them to get some context.

(Apologies to Rado for misreading his patch the first time!)

Radoslav Dorcik wrote:
> auto-detection of the plink/ssh implementation using -V option.

It looks like -p N is still being used.  All OpenSSH commands (ssh,
sftp, scp) support -oPort=N, so you might be able to remove that
distinction.

OTOH dbclient [DROPBEAR] accepts -p N, but neither -oPort=N nor -V.

[DROPBEAR] http://matt.ucc.asn.au/dropbear/dropbear.html

Have you had a chance to look at this, Guillaume? (trying to teach
myself to chase more)

Hi Radoslav,

Here goes my humble review, I have to say I'm no ssh expert nor
Windows user (and my remarks are hlint-esque),so my confidence is
quite low:

>+import System.IO (hGetContents)
>+import Control.Monad (foldM)
>+import Data.Maybe (fromJust)
>+import System.Process (waitForProcess)
>+
> import qualified Data.ByteString as B (ByteString, hGet, writeFile, readFile)
> import qualified Data.ByteString.Char8 as BC (unpack)
>
>hunk ./src/Ssh.hs 224
> getSSHOnly :: SSHCmd -> IO (String, [String])
> getSSHOnly cmd =
>  do ssh_command <- getEnv (evar cmd) `catchall` return (show cmd)
>-    -- port
>-    port <- (portFlag cmd `fmap` getEnv "SSH_PORT") `catchall` return []
>     let (ssh, ssh_args) = breakCommand ssh_command
>hunk ./src/Ssh.hs 225
>+    opts <- getSSHVerOpts cmd ssh

This is the call to the most important function which given some
command, tests which version is runnable, and gets the port flag
accordingly. I would have called the returned variable "sshOpts"
instead of just "opts".

>+    -- port
>+    port <- ((sshPortOpt opts) `fmap` getEnv "SSH_PORT") `catchall` return []


>     --
>hunk ./src/Ssh.hs 229
>-    return (ssh, ssh_args ++ port)
>+    return (ssh, ssh_args ++ port ++ (sshBatchFlag opts))

You could remove these parentheses around sshBatchFlag opts.

>     where
>      evar SSH  = "DARCS_SSH"
>      evar SCP  = "DARCS_SCP"
>hunk ./src/Ssh.hs 234
>      evar SFTP = "DARCS_SFTP"
>-     portFlag SSH  x = ["-p", x]
>-     portFlag SCP  x = ["-P", x]
>-     portFlag SFTP x = ["-oPort="++x]
>
> environmentHelpSsh :: ([String], [String])
> environmentHelpSsh = (["DARCS_SSH"], [
>hunk ./src/Ssh.hs 330
>
> (///) :: FilePath -> FilePath -> FilePath
> d /// f = d ++ "/" ++ f
>+
>+-- | Challenge request and function for the matching of challenge response
>+type SSHVersionDetect = ([String], String -> Bool)
>+
>+-- | Contains specific values for SSH implementations
>+--   like ssh, plink
>+data SSHCmdOpt = SSHCmdOpt { sshVerName    :: String                -- Information purpose
>+                           , sshPortOpt    :: String -> [String]
>+                           , sshBatchFlag  :: [String]
>+                           , sshQuietFlag  :: [String] }
>+
>+-- | Various implementations for `ssh` command
>+sshVersions :: [ (SSHVersionDetect,SSHCmdOpt) ]
>+sshVersions =  [ (plinkMatch       , SSHCmdOpt "plink"   (\p -> ["-P", p]) ["-batch"] [])
>+               , (sshMatch         , SSHCmdOpt "openssh" (\p -> ["-p", p]) []         ["-q"])
>+               , (defaultMatchAll  , SSHCmdOpt "default" (\p -> ["-p", p]) []         ["-q"]) ] -- Fallback to default
>+    where
>+      sshMatch        = (["-V"], (\s -> "OpenSSH" `isPrefixOf` s))
>+      plinkMatch      = (["-V"], (\s -> "plink:"  `isPrefixOf` s))
>+      defaultMatchAll = (["-V"], (\_ -> True))
>+
>+sftpVersions :: [ (SSHVersionDetect,SSHCmdOpt) ]
>+sftpVersions =  [ (defaultMatchAll  , SSHCmdOpt "default" (\p -> ["-oPort="++p]) [] []) ] -- Fallback to default
>+    where
>+      defaultMatchAll = (["-V"], (\_ -> True))
>+
>+scpVersions :: [ (SSHVersionDetect,SSHCmdOpt) ]
>+scpVersions =  [ (defaultMatchAll  , SSHCmdOpt "default" (\p -> ["-P", p]) [] ["-q"]) ] -- Fallback to default
>+    where
>+      defaultMatchAll = (["-V"], (\_ -> True))
>+
>+-- | Detect the type of the ssh command (ssh,plink,etc)
>+getSSHVerOpts :: SSHCmd -> String -> IO SSHCmdOpt
>+getSSHVerOpts cmdType cmd = do
>+  v <- (foldM (tryVersion) Nothing (cmdVersions cmdType))

You can remove the outer parentheses.

>+  return (fromJust v)
>+    where
>+      cmdVersions SSH  = sshVersions
>+      cmdVersions SFTP = sftpVersions
>+      cmdVersions SCP  = scpVersions
>+      tryVersion (Just o) _     = return (Just o)
>+      tryVersion Nothing  (d,o) = do
>+                          ret <- challengeCMD cmd (fst d)
>+                          if (snd d) ret then return (Just o) else return Nothing

OK I see what it does, trying to match the version string/prefix until it works.

>+
>+-- | Private function which takes command and arguments, execute and provides the
>+--   output (stderr+stdout) as the string
>+challengeCMD :: String -> [String] -> IO String
>+challengeCMD cmd args = do
>+  (_,outp,errp,pid) <- runInteractiveProcess cmd args Nothing Nothing
>+  outstr <- hGetContents outp
>+  errstr <- hGetContents errp
>+  waitForProcess pid
>+  let resp = (errstr++outstr)
>+  return resp

Looks good overall.

sorry for the delay!

guillaume

Thanks, Guillaume,

Comments on the review:

IMHO hlint style comments are surprisingly helpful, not so much
because they improve the code but because they improve the coder.
(That is, if the style bug comes from a subtle misunderstanding
of something deeper and not just a brain fart)
 
The do-I-understand-this-patch is what I call first level review,
(which is the level I'm at personally).  The sort of second-level review
which I aspire to is the kind where you start thinking harder about how
the patch interacts with the rest of the code (yay functional?).
Perhaps further up the ladder is to understand what the patch omits!
What is it failing to anticipate?  And further up the ladder are things
like fundamental design, maintainability, etc.  Phew.
   
This is why I think highly of public code-review as a sort of mental
exercise for programmers.  I guess the only way to get better at it is
to do a lot of reviewing (and read a lot of reviews).

On Thu, Jun 17, 2010 at 09:34:58 +0000, gh wrote:
>>hunk ./src/Ssh.hs 224
>> getSSHOnly :: SSHCmd -> IO (String, [String])
>> getSSHOnly cmd =
>>  do ssh_command <- getEnv (evar cmd) `catchall` return (show cmd)
>>-    -- port
>>-    port <- (portFlag cmd `fmap` getEnv "SSH_PORT") `catchall` return []
>>     let (ssh, ssh_args) = breakCommand ssh_command
>>hunk ./src/Ssh.hs 225
>>+    opts <- getSSHVerOpts cmd ssh
> 
> This is the call to the most important function which given some
> command, tests which version is runnable, and gets the port flag
> accordingly. I would have called the returned variable "sshOpts"
> instead of just "opts".

When is getSSHOnly called?

>>+-- | Contains specific values for SSH implementations
>>+--   like ssh, plink
>>+data SSHCmdOpt = SSHCmdOpt { sshVerName    :: String                -- Information purpose
>>+                           , sshPortOpt    :: String -> [String]
>>+                           , sshBatchFlag  :: [String]
>>+                           , sshQuietFlag  :: [String] }

Would it simplify the code any to put the SSHVersionDetect information
in SSHCmdOpt, or would that just be confusing design?

 , sshVersionInfoFlag :: [String]
 , sshMatches :: String -> Bool

>>+-- | Various implementations for `ssh` command
>>+sshVersions :: [ (SSHVersionDetect,SSHCmdOpt) ]
>>+sshVersions =  [ (plinkMatch       , SSHCmdOpt "plink"   (\p -> ["-P", p]) ["-batch"] [])
>>+               , (sshMatch         , SSHCmdOpt "openssh" (\p -> ["-p", p]) []         ["-q"])
>>+               , (defaultMatchAll  , SSHCmdOpt "default" (\p -> ["-p", p]) []         ["-q"]) ] -- Fallback to default
>>+    where
>>+      sshMatch        = (["-V"], (\s -> "OpenSSH" `isPrefixOf` s))
>>+      plinkMatch      = (["-V"], (\s -> "plink:"  `isPrefixOf` s))
>>+      defaultMatchAll = (["-V"], (\_ -> True))

>>+-- | Detect the type of the ssh command (ssh,plink,etc)
>>+getSSHVerOpts :: SSHCmd -> String -> IO SSHCmdOpt
>>+getSSHVerOpts cmdType cmd = do
>>+  v <- (foldM (tryVersion) Nothing (cmdVersions cmdType))
>>+  return (fromJust v)
>>+    where
>>+      cmdVersions SSH  = sshVersions
>>+      cmdVersions SFTP = sftpVersions
>>+      cmdVersions SCP  = scpVersions
>>+      tryVersion (Just o) _     = return (Just o)
>>+      tryVersion Nothing  (d,o) = do
>>+                          ret <- challengeCMD cmd (fst d)
>>+                          if (snd d) ret then return (Just o) else return Nothing
> 
> OK I see what it does, trying to match the version string/prefix until it works.

I wonder if Darcs.Util.firstJustIO would be useful here:

  http://www.darcs.net/api-doc/Darcs-Utils.html#v%3AfirstJustIO
 (which could perhaps be rewritten with foldM?)

I wish there was something we could do to make this a bit more self
evident. We have the three ideas:

 - stop on first success (firstJustIO?)
 - run ssh -V
 - apply the test on its output

which seems to be slightly obscured by the interplay between the tuple
unwrapping and the Maybe plumbing.

[I don't this this is a big problem, by the way... just wishing I knew
how to write clearer code!]

>>+-- | Private function which takes command and arguments, execute and provides the
>>+--   output (stderr+stdout) as the string
>>+challengeCMD :: String -> [String] -> IO String
>>+challengeCMD cmd args = do
>>+  (_,outp,errp,pid) <- runInteractiveProcess cmd args Nothing Nothing
>>+  outstr <- hGetContents outp
>>+  errstr <- hGetContents errp
>>+  waitForProcess pid
>>+  let resp = (errstr++outstr)
>>+  return resp

Do we have a deadlock problem?
http://www.haskell.org/pipermail/haskell-cafe/2008-May/042994.html
-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Thanks All for the comments!

1) Eric, the issue with possible deadlock I'll target in next version of
the patch.

2) getSSHOnly was found as the place where is decision taken about
ssh/sftp/scp command. it seems other commands use this for getting
command name / arguments. I'll re-verify that. 

3) these two "SSH CMD Opts" and "SSH CMD Version Detection" is good have
separated since there can be 1:N relation. 

4) the IO handling & re-implementation will be investigated, e.g.
incorporate robust approach.

I finally understood the problem why OpenSSH on Linux can take
interactive input / confirmation and windows putty can not - OpenSSH
access the terminal directly / bi-pass the stdin/stdout. This is quite
common practice for the commands which ask for the passwords. 

Unforunately I not able track the development on darcs, are there any
patches/related-issues for the SSH/SFTP Windows/Linux topic ?

Hmm, I think I must have stolen some ideas for the recent ssh work 
(patch687 in particular)

Could be worth taking that into account.

Obsoleted, according to dixie.