fix ncat sctp test and update CI image #978
Conversation
|
@edsantiago I hope this unblock us hopefully |
|
LGTM once it passes CI |
nmap-ncat seem to have weird bugs in that the received data is only printed on stdout when there is a no data on stdin, not stdin because the returns EOF when it gets read and if there is any data then ncat fails as well as it cannot write it to the remote[1]... So just try to emulate like how it works in a terminal by creating an anonymous pipe that contains no data so ncat is happy and prints our test string as expected. [1] nmap/nmap#2829 Signed-off-by: Paul Holzinger <[email protected]>
Contains a new ncat with udp bugs fixes, we still need the sctp workaround though. from containers/automation_images#349 Signed-off-by: Paul Holzinger <[email protected]>
Remove some unused ipva code fromt he dhcp proxy, I don't see us needing this anythime soon and we can always add it back later. Signed-off-by: Paul Holzinger <[email protected]>
|
@edsantiago @mheon PTAL again |
There was a problem hiding this comment.
Very weird behavior, see inline.
And your third commit (clippy) doesn't say what the clippy warnings actually are, and I'm assuming that "ipva" is a typo for "ipv6" maybe? Not a blocker from my perspective but I mention it for you to decide whether a future maintainer will care about that or not.
| @@ -644,7 +645,7 @@ function run_nc_test() { | |||
| fi | |||
|
|
|||
| nsenter -n -t "${CONTAINER_NS_PIDS[$container_ns]}" timeout --foreground -v --kill=10 5 \ | |||
| nc $nc_common_args -l -p $container_port &>"$NETAVARK_TMPDIR/nc-out" <$stdin & | |||
| nc $nc_common_args -l -p $container_port &>"$NETAVARK_TMPDIR/nc-out" <&$stdin & | |||
There was a problem hiding this comment.
Although CI is passing, it's not for the reasons described in your commit: $stdin is, at this point, undefined. I expected bash to treat this as an error, but it doesn't, and I'm completely lost as to what exactly is happening.
I think the answer here is to remove <&$stdin entirely, given that you've set up the exec, but I'm sorry I can't approve this as it is.
There was a problem hiding this comment.
sorry I don't follow why do you think $stdin is undefined?
There was a problem hiding this comment.
I will try to remove it, if that makes it work I happily do that as it makes the test code simpler
There was a problem hiding this comment.
$ cat <&
bash: syntax error near unexpected token `newline'
so I don't see why this would pass if it was actually undefined
There was a problem hiding this comment.
no wait hold on; I'm refreshing my memory on {varname}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
My bad. /lgtm |
see commits