On Nov 16, 2009, at 9:53 PM, Peter Stuge wrote:
> Dave McCaldon wrote:
>> I was hoping to get my patch from Nov. 4th into the code base:
>>
>> [PATCH] Map SSH_MSG_USERAUTH_FAILURE to LIBSSH2_ERROR_LOGIN_FAILED
>> in libssh2_userauth_password()
>
> Thanks for the reminder. When I first saw this my immediate reaction
> was that it was an improvement, but upon closer review I hesitate a
> little to commit it.
>
>
>> What's the correct process for submitting this kind of thing?
>
> I think you did the right thing sending it to the list. But please
> take care to send individual patches for logically grouped changes
> rather than squashed commits. (You may want to squash and that's
> fine, but please do it transparently before sending out patches.)
I think the squashed commit is because I used a branch locally to implement the
change.
> Also, note that the first line of the commit message is used as
> filename, as a single line description of your change in the git log,
> and in the shortlog on gitweb. Try to write a very concise
> description on the first line, not longer than 72 chars or so, then
> leave one blank line, and explain your commit further there.
Is it simply better to send a plain diff as others have done rather than a "git
commit by email"?
> Dave McCaldon wrote:
>> The patch adds a new error code, LIBSSH2_ERROR_LOGIN_FAILED which
>> is returned in the case of SSH_MSG_USERAUTH_FAILURE (this is the
>> same as the logic in libssh2_userauth_publickey_fromfile() that
>> handles an unrecognized public key).
>
> LOGIN_FAILED is very generic. The userauth_publickey function uses
> a very specific PUBLICKEY_UNVERIFIED return code for the same SSH
> layer error. I think that either the now suggested LOGIN_FAILED
> should be renamed to PASSWORD_NOT_ACCEPTED or similar, or both of
> these error branches should just return one and the same
> LIBSSH2_ERROR_ to the application - after all it is the same error
> code in the SSH protocol, and the application knows which function
> it just called. I don't see a big benefit with dividing the error
> code inside libssh2?
I chose LIBSSH2_ERROR_LOGIN_FAILED for two reasons:
1) As you point out, userath_publickey already returns a fairly
specific error
2) The login failure could be caused by either a bad password, and/or
invalid username
I can resubmit the patch introducing LIBSSH2_ERROR_AUTHORIZATION_FAILED as an
alias for LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED (-19) and have both functions now
return this value (preserving backward compatibility with current code),
although this presumably would alter the description for -19 to "Authorization
failed".
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
