[PATCH] Raindrops currently fails when provided a symlink to a socket. As this is a common practice for many deployment tools (Vlad, etc.) this patch adds support for finding the realpath prior to looking the socket up in /proc/net/unix

[PATCH] Raindrops currently fails when provided a symlink to a socket. As this is a common practice for many deployment tools (Vlad, etc.) this patch adds support for finding the realpath prior to looking the socket up in /proc/net/unix

[Brian Corrigan]

---
 lib/raindrops/linux.rb |    3 +++
 test/test_linux.rb     |   26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/lib/raindrops/linux.rb b/lib/raindrops/linux.rb
index fe2af09..a198253 100644
--- a/lib/raindrops/linux.rb
+++ b/lib/raindrops/linux.rb
@@ -8,6 +8,8 @@
 # Instead of snapshotting, Raindrops::Aggregate::LastDataRecv may be used
 # to aggregate statistics from +all+ accepted sockets as they arrive
 # based on the +last_data_recv+ field in Raindrops::TCP_Info
+require 'pathname'
+
 module Raindrops::Linux

   # The standard proc path for active UNIX domain sockets, feel free to call
@@ -41,6 +43,7 @@ module Raindrops::Linux
     else
       paths = paths.map do |path|
         path = path.dup
+        path = Pathname.new(path).realpath.to_s
         path.force_encoding(Encoding::BINARY) if defined?(Encoding)
         rv[path]
         Regexp.escape(path)
diff --git a/test/test_linux.rb b/test/test_linux.rb
index 65f25e0..81463c9 100644
--- a/test/test_linux.rb
+++ b/test/test_linux.rb
@@ -67,6 +67,32 @@ class TestLinux < Test::Unit::TestCase
     assert_equal 1, stats[tmp.path].queued
   end

+  def test_unix_resolves_symlinks
+    tmp = Tempfile.new("\xde\xad\xbe\xef") # valid path, really :)
+    File.unlink(tmp.path)
+    us = UNIXServer.new(tmp.path)
+
+    # Create a symlink
+    destination = Tempfile.new("somethingelse")
+    destination.unlink # We need an available name, not an actual file
+    link = File.symlink(tmp, destination)
+
+    @to_close << UNIXSocket.new(tmp.path)
+    stats = unix_listener_stats
+    assert_equal 0, stats[link.path].active
+    assert_equal 1, stats[link.path].queued
+
+    @to_close << UNIXSocket.new(tmp.path)
+    stats = unix_listener_stats
+    assert_equal 0, stats[link.path].active
+    assert_equal 2, stats[link.path].queued
+
+    @to_close << us.accept
+    stats = unix_listener_stats
+    assert_equal 1, stats[link.path].active
+    assert_equal 1, stats[link.path].queued
+  end
+
   def test_tcp
     s = TCPServer.new(TEST_ADDR, 0)
     port = s.addr[1]
-- 
1.7.0.4

Re: [PATCH] ...

[Eric Wong]

Brian Corrigan <brian@genexp.me> wrote:
> Subject: [PATCH] Raindrops currently fails when provided a
>  symlink to a socket. As this is a common practice for many deployment
>  tools (Vlad, etc.) this patch adds support for finding the realpath prior
>  to looking the socket up in /proc/net/unix

Erhm, I meant write the commit message with a separate short Subject:
line and body (like an email).  Most of the existing commits in
raindrops are written this way, and it's the standard for git.git.

I can rewrite the commit message if you don't have time, the rest
of the patch looks fine.  Thanks!

Re: [PATCH] ...

[Brian Corrigan]

Hey Eric - can you take it for here?  Also, thanks for the patience :).
Fwiw, it's cool to have code in this project.  I learned a lot by reviewing
the source.

On Tuesday, June 5, 2012, Eric Wong wrote:

> Brian Corrigan <brian@genexp.me <javascript:;>> wrote:
> > Subject: [PATCH] Raindrops currently fails when provided a
> >  symlink to a socket. As this is a common practice for many deployment
> >  tools (Vlad, etc.) this patch adds support for finding the realpath
> prior
> >  to looking the socket up in /proc/net/unix
>
> Erhm, I meant write the commit message with a separate short Subject:
> line and body (like an email).  Most of the existing commits in
> raindrops are written this way, and it's the standard for git.git.
>
> I can rewrite the commit message if you don't have time, the rest
> of the patch looks fine.  Thanks!
>

Re: [PATCH] ...

[Eric Wong]

Brian Corrigan <brian@genexp.me> wrote:
> Hey Eric - can you take it for here?  Also, thanks for the patience :).
> Fwiw, it's cool to have code in this project.  I learned a lot by reviewing
> the source.

Sure thing.  Btw, don't top post and don't send HTML parts.  There's a
lot of technical mailing lists that don't welcome these things.

Actually, did you get the test you added running at all?

File.symlink returns 0 on success, so using the return value is bogus.
I'll rewrite it an squash a patch on top of it.

Re: [PATCH] ...

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> Brian Corrigan <brian@genexp.me> wrote:
> > Hey Eric - can you take it for here?  Also, thanks for the patience :).
> > Fwiw, it's cool to have code in this project.  I learned a lot by reviewing
> > the source.
> 
> Sure thing.  Btw, don't top post and don't send HTML parts.  There's a
> lot of technical mailing lists that don't welcome these things.
> 
> Actually, did you get the test you added running at all?
> 
> File.symlink returns 0 on success, so using the return value is bogus.
> I'll rewrite it an squash a patch on top of it.

I'll squash the following on top of your changes.
However, I think the way unix_listener_stats behaves can pose
some confusing usability problems with symlinks

diff --git a/test/test_linux.rb b/test/test_linux.rb
index 81463c9..1da86cf 100644
--- a/test/test_linux.rb
+++ b/test/test_linux.rb
@@ -73,24 +73,24 @@ class TestLinux < Test::Unit::TestCase
     us = UNIXServer.new(tmp.path)
 
     # Create a symlink
-    destination = Tempfile.new("somethingelse")
-    destination.unlink # We need an available name, not an actual file
-    link = File.symlink(tmp, destination)
+    link = Tempfile.new("somethingelse")
+    File.unlink(link.path) # We need an available name, not an actual file
+    File.symlink(tmp.path, link.path)
 
     @to_close << UNIXSocket.new(tmp.path)
     stats = unix_listener_stats
-    assert_equal 0, stats[link.path].active
-    assert_equal 1, stats[link.path].queued
+    assert_equal 0, stats[tmp.path].active
+    assert_equal 1, stats[tmp.path].queued
 
-    @to_close << UNIXSocket.new(tmp.path)
+    @to_close << UNIXSocket.new(link.path)
     stats = unix_listener_stats
-    assert_equal 0, stats[link.path].active
-    assert_equal 2, stats[link.path].queued
+    assert_equal 0, stats[tmp.path].active
+    assert_equal 2, stats[tmp.path].queued
 
     @to_close << us.accept
     stats = unix_listener_stats
-    assert_equal 1, stats[link.path].active
-    assert_equal 1, stats[link.path].queued
+    assert_equal 1, stats[tmp.path].active
+    assert_equal 1, stats[tmp.path].queued
   end
 
   def test_tcp

-- 
Eric Wong

Re: [PATCH] ...

[Brian Corrigan]

On Tue, Jun 5, 2012 at 8:49 PM, Eric Wong <normalperson@yhbt.net> wrote:
> Eric Wong <normalperson@yhbt.net> wrote:
>> Brian Corrigan <brian@genexp.me> wrote:
>> > Hey Eric - can you take it for here?  Also, thanks for the patience :).
>> > Fwiw, it's cool to have code in this project.  I learned a lot by reviewing
>> > the source.
>>
>> Sure thing.  Btw, don't top post and don't send HTML parts.  There's a
>> lot of technical mailing lists that don't welcome these things.
>>
>> Actually, did you get the test you added running at all?
>>
>> File.symlink returns 0 on success, so using the return value is bogus.
>> I'll rewrite it an squash a patch on top of it.
>
> I'll squash the following on top of your changes.
> However, I think the way unix_listener_stats behaves can pose
> some confusing usability problems with symlinks
>
> diff --git a/test/test_linux.rb b/test/test_linux.rb
> index 81463c9..1da86cf 100644
> --- a/test/test_linux.rb
> +++ b/test/test_linux.rb
> @@ -73,24 +73,24 @@ class TestLinux < Test::Unit::TestCase
>     us = UNIXServer.new(tmp.path)
>
>     # Create a symlink
> -    destination = Tempfile.new("somethingelse")
> -    destination.unlink # We need an available name, not an actual file
> -    link = File.symlink(tmp, destination)
> +    link = Tempfile.new("somethingelse")
> +    File.unlink(link.path) # We need an available name, not an actual file
> +    File.symlink(tmp.path, link.path)
>
>     @to_close << UNIXSocket.new(tmp.path)
>     stats = unix_listener_stats
> -    assert_equal 0, stats[link.path].active
> -    assert_equal 1, stats[link.path].queued
> +    assert_equal 0, stats[tmp.path].active
> +    assert_equal 1, stats[tmp.path].queued
>
> -    @to_close << UNIXSocket.new(tmp.path)
> +    @to_close << UNIXSocket.new(link.path)
>     stats = unix_listener_stats
> -    assert_equal 0, stats[link.path].active
> -    assert_equal 2, stats[link.path].queued
> +    assert_equal 0, stats[tmp.path].active
> +    assert_equal 2, stats[tmp.path].queued
>
>     @to_close << us.accept
>     stats = unix_listener_stats
> -    assert_equal 1, stats[link.path].active
> -    assert_equal 1, stats[link.path].queued
> +    assert_equal 1, stats[tmp.path].active
> +    assert_equal 1, stats[tmp.path].queued
>   end
>
>   def test_tcp
>
> --
> Eric Wong

Cheers Eric!

-- 
Brian

[PATCH] unix_listener_stats follows and remembers symlinks

[Eric Wong]

Eric Wong <normalperson@yhbt.net> wrote:
> However, I think the way unix_listener_stats behaves can pose
> some confusing usability problems with symlinks

I think the following patch should improve usability with
symlinks and unix_listener_stats:

>From bd7236fe23c4388d2fa42a4f836aca3c796dabab Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Tue, 5 Jun 2012 17:49:43 -0700
Subject: [PATCH] unix_listener_stats follows and remembers symlinks

Teach unix_listener_stats to remember the symlink path
it followed and have it point to the same object as the
resolved (real) socket path.

This allows the case where looking up stats by symlinks
works if the symlink is given to unix_listener_stats:

  File.symlink("/real/path/of.sock", "/path/to/link.sock")
  stats = unix_listener_stats(["/path/to/link.sock"])
  stats["/path/to/link.sock"] => # same as stats["/real/path/of.sock"]
---
 lib/raindrops/linux.rb |  9 +++++++--
 test/test_linux.rb     | 20 +++++++++++---------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/lib/raindrops/linux.rb b/lib/raindrops/linux.rb
index a198253..1752b8a 100644
--- a/lib/raindrops/linux.rb
+++ b/lib/raindrops/linux.rb
@@ -43,9 +43,14 @@ module Raindrops::Linux
     else
       paths = paths.map do |path|
         path = path.dup
-        path = Pathname.new(path).realpath.to_s
         path.force_encoding(Encoding::BINARY) if defined?(Encoding)
-        rv[path]
+        if File.symlink?(path)
+          link = path
+          path = Pathname.new(link).realpath.to_s
+          rv[link] = rv[path] # vivify ListenerStats
+        else
+          rv[path] # vivify ListenerStats
+        end
         Regexp.escape(path)
       end
     end
diff --git a/test/test_linux.rb b/test/test_linux.rb
index 81463c9..a84eecf 100644
--- a/test/test_linux.rb
+++ b/test/test_linux.rb
@@ -73,24 +73,26 @@ class TestLinux < Test::Unit::TestCase
     us = UNIXServer.new(tmp.path)
 
     # Create a symlink
-    destination = Tempfile.new("somethingelse")
-    destination.unlink # We need an available name, not an actual file
-    link = File.symlink(tmp, destination)
+    link = Tempfile.new("somethingelse")
+    File.unlink(link.path) # We need an available name, not an actual file
+    File.symlink(tmp.path, link.path)
 
     @to_close << UNIXSocket.new(tmp.path)
     stats = unix_listener_stats
-    assert_equal 0, stats[link.path].active
-    assert_equal 1, stats[link.path].queued
+    assert_equal 0, stats[tmp.path].active
+    assert_equal 1, stats[tmp.path].queued
 
-    @to_close << UNIXSocket.new(tmp.path)
-    stats = unix_listener_stats
+    @to_close << UNIXSocket.new(link.path)
+    stats = unix_listener_stats([link.path])
     assert_equal 0, stats[link.path].active
     assert_equal 2, stats[link.path].queued
 
+    assert_equal stats[link.path].object_id, stats[tmp.path].object_id
+
     @to_close << us.accept
     stats = unix_listener_stats
-    assert_equal 1, stats[link.path].active
-    assert_equal 1, stats[link.path].queued
+    assert_equal 1, stats[tmp.path].active
+    assert_equal 1, stats[tmp.path].queued
   end
 
   def test_tcp
--