Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURLKHSTAT_FINE_REPLACE #5685

Closed
wants to merge 1 commit into from
Closed

CURLKHSTAT_FINE_REPLACE #5685

wants to merge 1 commit into from

Conversation

@mickae1
Copy link
Contributor

mickae1 commented Jul 15, 2020

add the functionality to remove the old host+key that doesn't match anymore. It's usefull to prevent the knownhost file to grow too much.

I've a problem during the compilation:

NMAKE : fatal error U1073: incapable d'obtenir '..\src\tool_hugehelp.c'

@deepcode-ci-bot
Copy link

deepcode-ci-bot bot commented Jul 15, 2020

Congratulations 🎉. DeepCode analyzed your code in 0.222 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

Copy link
Member

bagder left a comment

The libssh backend is also quite popular so I think it would make sense to make sure that is updated accordingly!

docs/libcurl/opts/CURLOPT_SSH_KEYFUNCTION.3 Outdated Show resolved Hide resolved
docs/libcurl/opts/CURLOPT_SSH_KEYFUNCTION.3 Outdated Show resolved Hide resolved
include/curl/curl.h Outdated Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jul 16, 2020

Don't forget make checksrc:

./vssh/libssh2.c:444:48: warning: Trailing whitespace (TRAILINGSPACE)
     struct ssh_conn *sshc = &conn->proto.sshc;  
                                                ^
./vssh/libssh2.c:590:53: warning: Trailing whitespace (TRAILINGSPACE)
         else if(rc == CURLKHSTAT_FINE_ADD_TO_FILE || 
                                                     ^
checksrc: 0 errors and 2 warnings
checksrc: 0 errors and 6 warnings suppressed

Also, please consider squashing the commits and force-pushing after extensive edits so that we can now do a fresh review on the entire thing a little easier.

@bagder bagder added the SCP/SFTP label Jul 18, 2020
@mickae1
Copy link
Contributor Author

mickae1 commented Jul 25, 2020

Ok, I will do it... In vacation for the moment

@mickae1 mickae1 force-pushed the mickae1:master branch from b70df3d to a78c0d5 Aug 3, 2020
@mickae1
Copy link
Contributor Author

mickae1 commented Aug 3, 2020

done :) waiting for the review ;)

@mickae1 mickae1 changed the title CURLKHSTAT_FINE_REPLACE_TO_FILE CURLKHSTAT_FINE_REPLACE Aug 3, 2020
Copy link
Member

bagder left a comment

The 1119 still fails.

The commit message mentions the wrong name and doesn't follow the style guide

docs/libcurl/opts/CURLOPT_SSH_KEYFUNCTION.3 Outdated Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
lib/vssh/libssh2.c Outdated Show resolved Hide resolved
@mickae1 mickae1 force-pushed the mickae1:master branch from 98b4709 to cb14980 Aug 3, 2020
@mickae1 mickae1 requested a review from bagder Aug 3, 2020
@bagder
Copy link
Member

bagder commented Aug 3, 2020

The 1119 test still fails. You need to add CURLKHSTAT_FINE_REPLACE to docs/libcurl/symbols-in-versions

@mickae1 mickae1 force-pushed the mickae1:master branch from 9819b69 to 5c7737a Aug 3, 2020
@mickae1
Copy link
Contributor Author

mickae1 commented Aug 3, 2020

The 1119 test still fails. You need to add CURLKHSTAT_FINE_REPLACE to docs/libcurl/symbols-in-versions

problem corrected :)

@mickae1 mickae1 force-pushed the mickae1:master branch from fed6fe7 to 86fd883 Aug 3, 2020
@mickae1
Copy link
Contributor Author

mickae1 commented Aug 6, 2020

@bagder requested changes

I've done the change, how can I say that it is done ?

@bagder
Copy link
Member

bagder commented Aug 6, 2020

You still forget to run make checksrc and several CI jobs pointed out this problem to you:

./vssh/libssh2.c:444:48: warning: Trailing whitespace (TRAILINGSPACE)
     struct ssh_conn *sshc = &conn->proto.sshc;  
                                                ^
./vssh/libssh2.c:590:53: warning: Trailing whitespace (TRAILINGSPACE)
         else if(rc == CURLKHSTAT_FINE_ADD_TO_FILE || 
                                                     ^
@mickae1
Copy link
Contributor Author

mickae1 commented Aug 6, 2020

You still forget to run make checksrc and several CI jobs pointed out this problem to you:

./vssh/libssh2.c:444:48: warning: Trailing whitespace (TRAILINGSPACE)
     struct ssh_conn *sshc = &conn->proto.sshc;  
                                                ^
./vssh/libssh2.c:590:53: warning: Trailing whitespace (TRAILINGSPACE)
         else if(rc == CURLKHSTAT_FINE_ADD_TO_FILE || 
                                                     ^

@bagder
sorry ... it's because, I'm on windows, and this doesn't work:

curl\winbuild> nmake checksrc ..\lib\vssh\libssh2.c

I've corrected the trailingspace 👍

@bagder
Copy link
Member

bagder commented Aug 6, 2020

I believe projects/checksrc.bat should be possible to invoke by windows users.

@mickae1
Copy link
Contributor Author

mickae1 commented Aug 6, 2020

projects/checksrc.bat

Thx,
@bagder

curl> projects/checksrc.bat .\lib\vssh\libssh2.c

no result from the command :)

looks like that it's okay 👍

@mickae1 mickae1 force-pushed the mickae1:master branch from cbe8cac to 713f81c Aug 6, 2020
lib/vssh/libssh2.c Show resolved Hide resolved
this functionality is usefull when you need to update the fingerprint of the host.
The other option : CURLKHSTAT_FINE_ADD_TO_FILE, append the new fingerprint in the file, but the old fingerprint is not deleted.
@mickae1 mickae1 force-pushed the mickae1:master branch from d609b43 to 8488411 Aug 6, 2020
@bagder
bagder approved these changes Aug 13, 2020
@bagder
Copy link
Member

bagder commented Aug 24, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.