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

Jump to bottom

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 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'

@ghost
Copy link

ghost commented Jul 15, 2020 •
edited by ghost

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

bagder
bagder reviewed Jul 15, 2020
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mickae1
Copy link
Contributor Author

mickae1 commented Jul 25, 2020

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

@bagder bagder added the feature-window A merge of this requires an open feature window label Jul 27, 2020
@mickae1
Copy link
Contributor Author

mickae1 commented Aug 3, 2020 •
edited

done :) waiting for the review ;)

@mickae1 mickae1 changed the title CURLKHSTAT_FINE_REPLACE_TO_FILE CURLKHSTAT_FINE_REPLACE Aug 3, 2020
bagder
bagder requested changes Aug 3, 2020
View reviewed changes
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 requested a review from bagder August 3, 2020 14:27
@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
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
Copy link
Contributor Author

mickae1 commented Aug 6, 2020 •
edited

@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 •
edited

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
SFTP: add the option CURLKHSTAT_FINE_REPLACE
8488411 
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.
@bagder bagder closed this in ebc6c54 Aug 24, 2020
@bagder bagder removed the feature-window A merge of this requires an open feature window label Aug 24, 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
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
SCP/SFTP
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mickae1 @bagder