[PATCH 1/2] Add setup and teardown for ipv6 tests

[PATCH 1/2] Add setup and teardown for ipv6 tests

[Hleb Valoshka]

---
 test/test_linux_ipv6.rb | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/test/test_linux_ipv6.rb b/test/test_linux_ipv6.rb
index ec08f28..9e8730a 100644
--- a/test/test_linux_ipv6.rb
+++ b/test/test_linux_ipv6.rb
@@ -12,6 +12,14 @@ class TestLinuxIPv6 < Test::Unit::TestCase
 
   TEST_ADDR = ENV["TEST_HOST6"] || "::1"
 
+  def setup
+    @to_close = []
+  end
+
+  def teardown
+    @to_close.each { |io| io.close unless io.closed? }
+  end
+
   def test_tcp
     s = TCPServer.new(TEST_ADDR, 0)
     port = s.addr[1]
-- 
1.8.4

[PATCH 2/2] Return real stats instead of True

[Hleb Valoshka]

---
 ext/raindrops/linux_inet_diag.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ext/raindrops/linux_inet_diag.c b/ext/raindrops/linux_inet_diag.c
index cd4a876..bae1202 100644
--- a/ext/raindrops/linux_inet_diag.c
+++ b/ext/raindrops/linux_inet_diag.c
@@ -620,11 +620,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 			return rv;
 		}
 		for (i = 0; i < len; i++) {
-			union any_addr check;
 			VALUE cur = rb_ary_entry(addrs, i);
 
-			parse_addr(&check, cur);
-			rb_hash_aset(rv, cur, Qtrue);
+			rb_hash_aset(rv, cur, tcp_stats(&args, cur));
 		}
 		/* fall through */
 	}
-- 
1.8.4

Re: [PATCH 2/2] Return real stats instead of True

[Eric Wong]

Hleb Valoshka <375gnu@gmail.com> wrote:

<no commit message body>

This deserves an explanation (and ideally, a test case).  I haven't
looked at the code in a while and needed to think a bit about
when the fix is relevant.

Thanks.

Re: [PATCH 2/2] Return real stats instead of True

[Hleb Valoshka]

On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:
> This deserves an explanation (and ideally, a test case).  I haven't
> looked at the code in a while and needed to think a bit about
> when the fix is relevant.

You better reject it. I've made it because of test failure:

Error: test_tcp_multi(TestLinuxIPv6)
  NoMethodError: undefined method `queued' for true:TrueClass
test/test_linux_ipv6.rb:54:in `test_tcp_multi'
     51:     addrs = [ addr1, addr2 ]
     52:     stats = tcp_listener_stats(addrs)
     53:     assert_equal 2, stats.size
  => 54:     assert_equal 0, stats[addr1].queued
     55:     assert_equal 0, stats[addr1].active
     56:     assert_equal 0, stats[addr2].queued
     57:     assert_equal 0, stats[addr2].active

With that changes test's passed. (I didn't noticed that time that you
process that Qtrues).

But the real reason may be something else: I've got IPv6 address using
Teredo and getnameinfo returns error with it. So it cause other
errors, I need to check it better.

Re: [PATCH 2/2] Return real stats instead of True

[Eric Wong]

Hleb Valoshka <375gnu@gmail.com> wrote:
> On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:
> > This deserves an explanation (and ideally, a test case).  I haven't
> > looked at the code in a while and needed to think a bit about
> > when the fix is relevant.
> 
> You better reject it. I've made it because of test failure:

Ah, thanks.  I've been forgetting to test on IPv6-enabled systems :x

(I typically run custom kernels without IPv6, there were
 compatibility/slowdown issues (way) back in the day and it was
 easier to just not enable IPv6 at all.
 Probably moot at this point...)

> But the real reason may be something else: I've got IPv6 address using
> Teredo and getnameinfo returns error with it. So it cause other
> errors, I need to check it better.

OK.

Re: [PATCH 2/2] Return real stats instead of True

[Hleb Valoshka]

On 9/9/13, Eric Wong <normalperson@yhbt.net> wrote:

>> But the real reason may be something else: I've got IPv6 address using
>> Teredo and getnameinfo returns error with it. So it cause other
>> errors, I need to check it better.
> OK.

Using some debug output I've found the reason (it's not a teredo, it
was disabled).

Lets look into linux_inet_diag.c, if tcp_listener_stats has only 1 arg, you do

rb_hash_aset(rv, addrs, tcp_stats(&args, addrs));
(or cur = rb_ary_entry(addrs, 0);
rb_hash_aset(rv, cur, tcp_stats(&args, cur)); )
return rv;

But if it has more agrs, you do

    for (i = 0; i < len; i++) {
      union any_addr check;
      VALUE cur = rb_ary_entry(addrs, i);

      parse_addr(&check, cur);
      rb_hash_aset(rv, cur, Qtrue);

and then fallback to branch where arg is nil, the most interesting
thing is calling of st_AND_hash(key, stats, hash) which has

 if (rb_hash_lookup(hash, key) == Qtrue) {
      VALUE v = rb_listen_stats(stats);
      OBJ_FREEZE(key);
      rb_hash_aset(hash, key, v);
    }

The problem is `key'. It may differ from what you expect.

For example, in tests you do
TEST_ADDR = ENV["TEST_HOST6"] || "::1"
addr1, addr2 = "[#{TEST_ADDR}]:#{port1}", "[#{TEST_ADDR}]:#{port2}"

In my last case they are [::1]:42244, [::1]:46335, but values of `key'
for them are:
[::1%3336]:42244, [::1%3336]:46335, so rb_hash_lookup never returns Qtrue.

Re: [PATCH 2/2] Return real stats instead of True

[Eric Wong]

Thanks for the analysis.  Can you send a fix?  Otherwise I'll try to
take care of it within 2 weeks (going on vacation in a few days :)

Re: [PATCH 2/2] Return real stats instead of True

[Hleb Valoshka]

On 9/10/13, Eric Wong <normalperson@yhbt.net> wrote:

> Thanks for the analysis.  Can you send a fix?  Otherwise I'll try to
> take care of it within 2 weeks (going on vacation in a few days :)

OK, I'll try

Re: [PATCH 1/2] Add setup and teardown for ipv6 tests

[Eric Wong]

Oops, forgot this trivial patch.  Applied and pushed.
New release + mailing list coming soon.  Thanks.

Re: [PATCH 2/2] Return real stats instead of True

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Hleb Valoshka <375gnu@gmail.com> wrote:
> 
> <no commit message body>
> 
> This deserves an explanation (and ideally, a test case).  I haven't
> looked at the code in a while and needed to think a bit about
> when the fix is relevant.

Resurrecting this from the old list 2.5 years ago:
http://bogomips.org/raindrops-public/20130909192020.GA18010@dcvr.yhbt.net/t/#u

I actually found a different reason for this problem.  Normally,
I never change the set of listeners.  But lately I've been
switching between backend HTTP servers that needed to run over TCP
(as opposed to Unix sockets) and dropping listeners occasionally
was causing "true" to show up in the results.

-----------8<------------
Subject: [PATCH] linux: tcp_listener_stats drops "true" placeholders

With invalid addresses specified which give no currently-bound
address, we must avoid leaving placeholders ('true' objects)
in our results.

Clean up some shadowing "cur" while we're at it.
---
 ext/raindrops/linux_inet_diag.c | 15 ++++++++++++---
 test/test_linux.rb              |  7 +++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/ext/raindrops/linux_inet_diag.c b/ext/raindrops/linux_inet_diag.c
index 35bb127..58415c6 100644
--- a/ext/raindrops/linux_inet_diag.c
+++ b/ext/raindrops/linux_inet_diag.c
@@ -618,6 +618,13 @@ static VALUE tcp_stats(struct nogvl_args *args, VALUE addr)
 	return rb_listen_stats(&args->stats);
 }
 
+static int drop_placeholders(st_data_t k, st_data_t v, st_data_t ign)
+{
+	if ((VALUE)v == Qtrue)
+		return ST_DELETE;
+	return ST_CONTINUE;
+}
+
 /*
  * call-seq:
  *      Raindrops::Linux.tcp_listener_stats([addrs[, sock]]) => hash
@@ -658,10 +665,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 	case T_ARRAY: {
 		long i;
 		long len = RARRAY_LEN(addrs);
-		VALUE cur;
 
 		if (len == 1) {
-			cur = rb_ary_entry(addrs, 0);
+			VALUE cur = rb_ary_entry(addrs, 0);
 
 			rb_hash_aset(rv, cur, tcp_stats(&args, cur));
 			return rv;
@@ -671,7 +677,7 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 			VALUE cur = rb_ary_entry(addrs, i);
 
 			parse_addr(&check, cur);
-			rb_hash_aset(rv, cur, Qtrue);
+			rb_hash_aset(rv, cur, Qtrue /* placeholder */);
 		}
 		/* fall through */
 	}
@@ -689,6 +695,9 @@ static VALUE tcp_listener_stats(int argc, VALUE *argv, VALUE self)
 	st_foreach(args.table, NIL_P(addrs) ? st_to_hash : st_AND_hash, rv);
 	st_free_table(args.table);
 
+	if (RHASH_SIZE(rv) > 1)
+		rb_hash_foreach(rv, drop_placeholders, Qfalse);
+
 	/* let GC deal with corner cases */
 	if (argc < 2) rb_io_close(sock);
 	return rv;
diff --git a/test/test_linux.rb b/test/test_linux.rb
index 0e79a86..bfefcc4 100644
--- a/test/test_linux.rb
+++ b/test/test_linux.rb
@@ -214,6 +214,13 @@ def test_tcp_multi
     assert_equal 0, stats[addr1].active
     assert_equal 1, stats[addr2].queued
     assert_equal 1, stats[addr2].active
+
+    # make sure we don't leave "true" placeholders in results if a
+    # listener becomes invalid (even momentarily).
+    s2.close
+    stats = tcp_listener_stats(addrs)
+    assert stats.values.all? { |x| x.instance_of?(Raindrops::ListenStats) },
+      "placeholders left: #{stats.inspect}"
   end
 
   # tries to overflow buffers
-- 
EW