Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 69.60% 70.10% +0.50%
==========================================
Files 17 17
Lines 2625 2572 -53
==========================================
- Hits 1827 1803 -24
+ Misses 798 769 -29 |
|
ci:rerun |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-292/ This is a comment from an automated CI system. Warnings Generated during build:Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: |
| if (!ssh_socket->config.host) | ||
| goto error; | ||
| ssh_socket->config.port = config->port; | ||
|
|
||
| ssh_socket->config.username = lrtr_strdup(config->username); | ||
| if (!ssh_socket->config.username) | ||
| goto error; | ||
|
|
||
| if ((config->password && config->client_privkey_path) || (!config->password && !config->client_privkey_path)) | ||
| return TR_ERROR; | ||
|
|
||
| if (config->bindaddr) | ||
| if (config->bindaddr) { | ||
| ssh_socket->config.bindaddr = lrtr_strdup(config->bindaddr); | ||
| else |
There was a problem hiding this comment.
why is the logic different for config->host and config->username compared to e.g. config->bindaddr. The latter is checked first then duplicated the others are duplicated first then checked. IMHO this should be handled in a consistent fashion.
There was a problem hiding this comment.
bindaddr is optional. We must first check if it is set and than if the strdup was successful.
tools/rtrclient.c
Outdated
| continue; | ||
| } | ||
|
|
||
| // if none of the conditions matched. The string contains at least one character that are not valid utf8 |
tools/rtrclient.c
Outdated
| size_t len = strlen(str); | ||
|
|
||
| for (size_t i = 0; i <= len; ++i) { | ||
| // check if current byte is a single utf char |
There was a problem hiding this comment.
minor: /utf/utf8/ to be consistent with the following comments
tools/rtrclient.c
Outdated
| } | ||
|
|
||
| // if none of the conditions matched. The string contains at least one character that are not valid utf8 | ||
| return false; |
There was a problem hiding this comment.
putting this into an else branch would make all continue statements in all other branches obsolete - but would also make the first if an empty statement. However, I personally would avoid continue for better readability. Maybe rewrite the loop.
There was a problem hiding this comment.
int i = 0;
bool is_utf8 = true;
while (i < len && is_utf8) {
if (//1 byte) {
i += 1;
}
else if (//2 bytes) {
i += 2;
}
...
else {
is_utf8 = false;
}
}
return is_utf8;
There was a problem hiding this comment.
however, just a suggestion not a blocker
There was a problem hiding this comment.
I would argue that the current form is more readable, because the continue makes it clear that there is nothing relevant after the if.
There was a problem hiding this comment.
I still disagree, all continues are only required bc return false is not in an else branch. A curious developer will tend to look what is skipped over by continue anyway - and will discover its only the return false. I'm not against using continue in general, but here it seems unnecessary to me.
Further, I think it is suboptimal that i is manipulated twice, i.e., in the loop statement (++i) and by all else if cases. Also it is counter intuitive that i is increase by byte-size - 1 - I admit that the for-loop avoids accidentally creating an invite loop, hence I still find something like
int i = 0;
while (i < len) {
if (// 1 byte utf8) {
//do stuff
i += 1;
}
else if (//2 byte utf8) {
// do stuff
i += 2;
}
...
else {
return false;
}
}
return true;
easier to understand and less error prone.
There was a problem hiding this comment.
I'm starting to think that you are right, changed it accordingly.
tools/rtrclient.c
Outdated
| if ((str[i] & UTF8_ONE_BYTE_MASK) == UTF8_ONE_BYTE_PREFIX) { | ||
| continue; | ||
|
|
||
| // check if current byte is the start of a two byte utf8 char and validate subsequent bytes |
There was a problem hiding this comment.
minor: indention of comment not aligned like comment above? Same for next 2 else if.
This comment has been minimized.
This comment has been minimized.
tools/rtrclient.c
Outdated
| { | ||
| size_t len = strlen(str); | ||
|
|
||
| for (size_t i = 0; i <= len; ++i) { |
There was a problem hiding this comment.
shouldn't this be i < len instead of i <= len?
|
I also noticed two additional problems myself.
|
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedFreeBSD 11 amd64: Failed (click for details)Make Debug failed for FreeBSD 11 amd64: Successful on other platforms/tests
Warnings Generated during build:Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-327/ This is a comment from an automated CI system. Warnings Generated during build:Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-328/ This is a comment from an automated CI system. Warnings Generated during build:Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-329/ This is a comment from an automated CI system. Warnings Generated during build:Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: |
|
ping @pguibert6WIND |
|
ping @pguibert6WIND |
This comment has been minimized.
This comment has been minimized.
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-336/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 18.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 i386: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Debian 11 amd64: Successful with additional warningsDebian Package lintian failed for Debian 11 amd64: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: |
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedUbuntu 16.04 arm7: Failed (click for details)Ubuntu 16.04 arm7: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Debian 11 amd64: Successful with additional warningsDebian Package lintian failed for Debian 11 amd64: Ubuntu 18.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 i386: Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: |
It does not make much sense to set both in non interactive authentication, but one of them has to be set.
libssh has deprecated ssh_is_known_server, ssh_session_is_known_server should be used instead. It is available since version 0.8.0, we will use it if available.
The timeout in the tr_ssh_recv implementation was hard coded to 1 second, instead of using the provided timeout.
…nnel_select ssh_channel_select can return SSH_EINTR, SSH_ERROR and SSH_OK. Handle SSH_EINTR and SSH_ERROR accordingly and continue for SSH_OK.
Some functions returned SSH_* error values, instead of TR_* error values
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/RPKI-RTRLIBPR-338/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64: Ubuntu 16.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm8: Ubuntu 18.04 ppc64le: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 ppc64le: Ubuntu 18.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 i386: Ubuntu 18.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 amd64: Ubuntu 16.04 i386: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 i386: Debian 11 amd64: Successful with additional warningsDebian Package lintian failed for Debian 11 amd64: Ubuntu 16.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 amd64: Ubuntu 16.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 16.04 arm7: Ubuntu 18.04 arm7: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm7: Ubuntu 14.04 amd64: Successful with additional warningsDebian Package lintian failed for Ubuntu 14.04 amd64: Ubuntu 20.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 20.04 arm8: Ubuntu 18.04 arm8: Successful with additional warningsDebian Package lintian failed for Ubuntu 18.04 arm8: |
Contribution description
Several fixes for the ssh transport
Issues/PRs references
fixes #212
fixes #250