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

tests: Improve hitch_hosts(), add configure checks #234

Closed
wants to merge 3 commits into from
Closed

tests: Improve hitch_hosts(), add configure checks #234

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 2, 2017

This fixes hitch_hosts() on OpenBSD where fstat(1) or netstat(1) from
base are used in such cases.

Follow up to #209.

This fixes hitch_hosts() on OpenBSD where fstat(1) or netstat(1) from
base are used in such cases.
@dridi
Copy link
Member

dridi commented Dec 12, 2017

With this pull request, we now have 3 inconsistent uses of awk and sed (and I'm responsible for 2 of them). Since you already agreed to add the configure checks ensuring that we have at least one of each, I won't bother you with code style for which I'm guiltier.

The code looks good to me, in the mean time I'll try to test it on OpenBSD to get familiar-enough with fstat in case of future changes in this area.

Thanks!

@ghost
Copy link
Author

ghost commented Dec 12, 2017

I omitted unifying hunks in my diff since I couldn't test them on other platforms. Maybe I can do so later on and amend here.

Klemens Nanni added 2 commits December 12, 2017 22:17
Simplify lsof usage, fix sockstat usage to look for Hitch's PID instead
of processes named "tcp".

Unify filtering across all tools with a single awk script, this makes
the code more readable and easier to adapt in case of changes.

Limiting fstat to IPv4 avoids printing two lines in case hitch listens
on both address families.

The filters only (without running the entire test suite) have been
tested on Debian and OpenBSD.
@ghost ghost changed the title tests: Try fstat in hitch_hosts() tests: Improve hitch_hosts(), add configure checks Dec 12, 2017
@ghost
Copy link
Author

ghost commented Dec 12, 2017

Just added the configure bits and made the filters consistent across different tools.

Please run the test suite with this on your machines if you can, I will do so tomorrow on CentOS and OpenBSD.

@dridi
Copy link
Member

dridi commented Dec 13, 2017

Doesn't work for me, tests fail.

$ uname -a
OpenBSD localhost.localdomain 6.2 GENERIC.MP#134 amd64

OpenBSD doesn't seem to have pathchk so when I comment those out I'm down to two test failures:

  • FAIL: tests/test03-multiple-listen (very much related to this patch)
  • FAIL: tests/test14-ocsp-vfy (looks as if SSL_CERT_FILE is ignored)

I started from this pull request's branch, rebased against master, produced a clean make dist archive, and tried to build that archive on OpenBSD.

@ghost
Copy link
Author

ghost commented Dec 13, 2017

pathchk is another issue on my todo list, yes; it's not available on OpenBSD.

Well, what does test-suite.log say? You're most likely running into unrelated issues here.

My test results look good so far, nothing changes/breaks.

dridi added a commit that referenced this pull request Dec 13, 2017
The original patch set from Klemens Nanni <[email protected]> added support
for fstat and while reviewing it we soon realized that the Linux port for
sockstat threw away any possible attempt at portability. As it turns out
FreeBSD also supports fstat and is portable enough to satisfy our testing
needs for both OpenBSD and FreeBSD.

Refs #234
dridi added a commit that referenced this pull request Dec 13, 2017
Not available on OpenBSD, only in POSIX... Commands relying on the paths
formerly checked will certainly choke on invalid paths.

Refs #234
@dridi
Copy link
Member

dridi commented Dec 13, 2017

207a382 and c8cbd67 are the results of discussions on IRC instead of here.

dridi added a commit that referenced this pull request Dec 13, 2017
FreeBSD's fstat doesn't print listen addresses in the NAME column.

Refs #234
dridi added a commit that referenced this pull request Dec 13, 2017
@dridi
Copy link
Member

dridi commented Dec 13, 2017

Today I tried to support sockstat on Linux too and I'm offensively putting it in the "garbage until proven otherwise" category. It's available on Debian 9, and to put it simply doesn't work:

vagrant@stretch:~$ nc -p 8080 -l 127.0.0.1 &
[1] 11901
vagrant@stretch:~$ lsof -a -i TCP -P -p $(jobs -p)
COMMAND   PID    USER   FD   TYPE DEVICE SIZE/OFF NODE NAME
nc      11901 vagrant    3u  IPv4 217461      0t0  TCP *:8080 (LISTEN)
vagrant@stretch:~$ sockstat -P $(jobs -p)
USER     PROCESS    PID      PROTO  SOURCE ADDRESS  FOREIGN ADDRESS  STATE
vagrant  nc         11901    tcp4   *:7936          *:*              LISTEN

This off-by-144 is a mystery, so that's now settled: lsof if available, sockstat on FreeBSD and fstat on OpenBSD. We can revisit that if someone like @klenan knocks at our bugtracker for another OS.

The project looks dead by the way:

$ dpkg-query -s sockstat | grep Homepage
Homepage: http://nenolod.net/sockstat

Unless it's only me that can't reach http://nenolod.net/ at all.

Now the reason why this ticket is still open is because one test is failing on OpenBSD, the one test making use of the SSL_CERT_FILE environment variable. I'm clueless and currently suspect a difference in LibReSSL.

@dridi
Copy link
Member

dridi commented Dec 15, 2017

The SSL_CERT_* environment variables seem to be supported by openssl 1.1.0+ which would explain why OpenBSD doesn't support it currently. I will try again later with Debian 8/Ubuntu or CentOS 6/7 to confirm this but I find it surprising that we'd never seen it fail there before.

@ghost ghost closed this Jan 16, 2018
@ghost ghost deleted the fstat branch January 16, 2018 15:57
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant