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

sshserver.pl fixes #715

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Karlson2k
Contributor

Karlson2k commented Mar 15, 2016

Testsuite running SSH script fixes.

Karlson2k added some commits Feb 22, 2016

sshserver.pl: Fixed failed redirection of stderr with some options.
At least on Msys2, perl fails to redirect stderr if $value contains
newline or other weird characters.
@mention-bot

This comment has been minimized.

mention-bot commented Mar 15, 2016

By analyzing the blame information on this pull request, we identified @yangtse, @mback2k and @dfandrich to be potential reviewers

@bagder

This comment has been minimized.

Member

bagder commented Mar 16, 2016

AuthorizedKeysFile2 was removed in OpenSSH 5.9, released in September 2011. I say we just stop using that option completely everywhere. Agree?

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Mar 18, 2016

@bagder Agree.
Update this PR?

@bagder bagder closed this in 2716350 Mar 20, 2016

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Mar 22, 2016

I don't know if this is related but a bunch of autobuilds seem to be failing, as of this commit, due to issues starting the SSH server:

  • Dan Fandrich's torture tests
  • My Ubuntu 14.04 and 15.04 builds but not my 15.10 builds

I haven't looked at the problem in detail but just wanted to make you aware of the problem before release.

@bagder

This comment has been minimized.

Member

bagder commented Mar 22, 2016

Thanks! Hm. A quick google tells me ubuntu 14.04 has a newer openssh server than 5.9 and thus support for that config option was already removed then.

But ok, let me revert that for now and see if we get back some working builds.

@bagder

This comment has been minimized.

Member

bagder commented Mar 22, 2016

commit d5e7f50 reverted the commit

@bagder bagder reopened this Mar 22, 2016

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Mar 22, 2016

@bagder This PR shouldn't break anything.
However, strange that removing usage of undocumented option breaks something.

@bagder

This comment has been minimized.

Member

bagder commented Mar 22, 2016

I agree, but since something broke I'm testing it out by reverting this...

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Mar 22, 2016

Ironically, with fantastic timing, it looks like my autobuild VMs have gone down - my email server is still up so I'm not sure what is wrong at the moment :(

Hopefully Dan's tests will run over night.

@captain-caveman2k

This comment has been minimized.

Member

captain-caveman2k commented Mar 22, 2016

It turns out I had filled up my mailbox on my mail server - deleting some of the emails has enabled my autobuild emails to go out now.

As such, you can see that the 14:30 Ubuntu 14.04 and 16:30 15:04 are now passing as they did previously. I don't know why this fix affected them as it did - but I will try and investigate at the weekend.

Unfortunately I only have 15.10 on my laptop which wasn't affected by the change.

@bagder

This comment has been minimized.

Member

bagder commented Mar 22, 2016

Thanks @captain-caveman2k . So @Karlson2k, what was your original reason for removing this option? Was it something that failed to work properly with it still present?

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Mar 23, 2016

@bagder No, it was just annoying strings in logs.
Pay attention to second commit. It helps not only with stderr, but also with any parameter with space or tab.

@bagder

This comment has been minimized.

Member

bagder commented Mar 26, 2016

Pay attention to second commit. It helps not only with stderr, but also with any parameter with space or tab.

And we have a problem with that?

@bagder

This comment has been minimized.

Member

bagder commented Mar 26, 2016

That quote fix is now in commit e326448. If I may ask, please read https://curl.haxx.se/dev/contribute.html#Write_good_commit_messages for a small "guideline" on how we prefer our commit messages. It would make me merge your fixes even faster!

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Mar 27, 2016

Pay attention to second commit. It helps not only with stderr, but also with any parameter with space or tab.

And we have a problem with that?

If any path is passed in parameter - it may be misinterpreted as several parameters as path may contain spaces.

@Karlson2k

This comment has been minimized.

Contributor

Karlson2k commented Mar 27, 2016

If I may ask, please read https://curl.haxx.se/dev/contribute.html#Write_good_commit_messages for a small "guideline" on how we prefer our commit messages.

I read it already, but it isn't easy to keep all different preferences for different project in my mind.
Will try better! 😄

@bagder

This comment has been minimized.

Member

bagder commented Mar 31, 2016

As that old config option doesn't seem to harm anyone but caused trouble when we tried to remove it, let's leave it there a few more years and then do another attempt! case closed for this time.

@bagder bagder closed this Mar 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment