On Oct 17, 2006, at 7:24 PM, Dan Fandrich wrote:

> On Tue, Oct 17, 2006 at 06:31:33PM -0400, James Housley wrote:
>> I have taken most of your feed back from the first patch set.
>
> A few more comments:
>
> - The CURLOPT identifiers, which form a part of the curl API,
> should not
> include the specific SSH implementation being used by libcurl. e.g.
> instead of CURLOPT_LIBSSH2_AUTH_TYPES call it CURLOPT_SSH_AUTH_TYPES,
> so that it stays valid if someone adds support for an alternative SSH
> implementation in the future (such as has happened for SSL).

Okay, I will do that.

>
> - Code has been added to auto-detect sftp and scp transfers by
> looking at
> host names beginning with sftp and scp. Are there really hosts out
> there
> like scp.example.com or sftp.example.net? If so, then the checks
> should
> really include a dot (matching 'scp.') so that someone trying to
> download
> the home page of the Center for Surveillance, Control and
> Prevention of
> Hand Injuries (curl scphi.com) gets an HTTP download instead of an SCP
> one. (I think the similiar check for "ftps" instead of "ftps."
> should be
> changed, too; the Fuller Termite & Pest Solutions, Inc. people
> might start to
> complain.)

If you mean this code
Index: lib/url.c
+#ifdef USE_LIBSSH2
+ else if(checkprefix("SFTP", conn->host.name))
+ strcpy(conn->protostr, "sftp");
+ else if(checkprefix("SCP", conn->host.name))
+ strcpy(conn->protostr, "scp");
+#endif /* USE_LIBSSH2 */

I will remove it, I was just trying to mimic ftp/ftps as best as I
could.

>
> - There are a few places where HAVE_LIBSSH2 is checked, but it
> probably
> ought to be USE_LIBSSH2
>

I will look through that more. What is the difference between HAVE_
and USE_ since DISABLE_ doesn't make sense because you have to
explicity add LIBSSH2

> - The code includes the comment "This implementation ignores the host
> name in conformance with RFC 1738. Only local files..." which looks
> to me
> like a cut-and-paste problem. There are also several other references
> to file: URLs that should be removed.

That was in starter code I was given. But I will look at it.

>
> - Does libssh2 really support pkg-config? The packages I've seen
> don't
> include pkg-config files.

??? Not sure what you mean here.

>
> - CURLVERSION_FORTH should be CURLVERSION_FOURTH
>

Of course you are right.

>> curl -O scp://housley@baby:~/rc.conf <-- works
>
> Shouldn't this URL be scp://housley@baby/~/rc.conf ?

Yup, that is what I meant.

Jim

--
/"\   ASCII Ribbon Campaign  .
\ / - NO HTML/RTF in e-mail  .
  X  - NO Word docs in e-mail .
/ \ -----------------------------------------------------------------
jeh@FreeBSD.org      http://www.FreeBSD.org     The Power to Serve
jim@TheHousleys.Net  http://www.TheHousleys.net
---------------------------------------------------------------------
In theory there is no difference between theory and practice.
In practice there is no similarity.
       -- From the "I wish I'd said that" archives.