From: Laurent Julliard <laurent@...>
Date: 2008-03-16T17:12:13+09:00
Subject: Re: How to make net-ping thread safe?

Joel VanderWerf wrote:
> Joel VanderWerf wrote:
>> In icmp.rb, the problem seems to be that it uses process ID as the 
>> ICMP packet ID, so it is impossible to tell which ECHO REPLY packet 
>> corresponds to which request (if multiple requests come from one 
>> process).
> 
> On second thought, the problem could be solved in icmp.rb by using the 
> sequence number to separate requests, and keeping the sequence number in 
> an ICMP class var instead of instance vars. Here's a patch that seems to 
> make your original code work correctly (detecting some hosts up and some 
> down).
> 

Joel,

Fist of all thanks for catching this!

Your recommendation is to turn the sequence number into a class variable 
so shouldn't the sequence number really be named @@seq rather than @seq 
if we want the sequence number to be unique across multiple instances of 
ICMP objects?

And if this is so, one of the consequence is that for a given ICMP 
instance chances are that the seq field in the icmp packet will not 
contain consecutive values. I don't know if this is a problem wrt to the 
ICMP specifications or not.

Don't you think an alternative would be not to use the PID as the packet 
identifier but rather a class variable of our own that we would 
increment each time a new ICMP instance is created?

Thanks again for your comments.

Laurent



> --- 
> /usr/local/lib/ruby/gems/1.8/gems/net-ping-1.2.2/lib/net/ping/icmp.rb    
> 2008-01-23 19:46:14.000000000 -0800
> +++ net/ping/icmp.rb    2008-03-15 13:09:55.000000000 -0700
> @@ -24,7 +24,6 @@
>        def initialize(host=nil, port=nil, timeout=5)
>           raise 'requires root privileges' if Process.euid > 0
> 
> -         @seq = 0
>           @bind_port = 0
>           @bind_host = nil
>           @data_size = 56
> @@ -37,6 +36,11 @@
>           super(host, port, timeout)
>           @port = nil # This value is not used in ICMP pings.
>        end
> +
> +      @seq = 0
> +      def self.next_seq
> +        @seq = (@seq + 1) % 65536
> +      end
> 
>        # Sets the number of bytes sent in the ping method.
>        #
> @@ -73,14 +77,14 @@
>              socket.bind(saddr)
>           end
> 
> -         @seq = (@seq + 1) % 65536
> +         seq = ICMP.next_seq
>           pstring = 'C2 n3 A' << @data_size.to_s
>           timeout = @timeout
> 
>           checksum = 0
> -         msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq, 
> @data].pack(pstring)
> +         msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq, 
> @data].pack(pstring)
>           checksum = checksum(msg)
> -         msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, @seq, 
> @data].pack(pstring)
> +         msg = [ICMP_ECHO, ICMP_SUBCODE, checksum, @pid, seq, 
> @data].pack(pstring)
> 
>           start_time = Time.now
> 
> @@ -101,7 +105,7 @@
>                 end
> 
>                 pid = nil
> -               seq = nil
> +               rcv_seq = nil
> 
>                 data, sender  = socket.recvfrom(1500)
>                 port, host    = Socket.unpack_sockaddr_in(sender)
> @@ -110,15 +114,15 @@
>                 case type
>                    when ICMP_ECHOREPLY
>                       if data.length >= 28
> -                        pid, seq = data[24, 4].unpack('n3')
> +                        pid, rcv_seq = data[24, 4].unpack('n3')
>                       end
>                    else
>                       if data.length > 56
> -                        pid, seq = data[52, 4].unpack('n3')
> +                        pid, rcv_seq = data[52, 4].unpack('n3')
>                       end
>                 end
> 
> -               if pid == @pid && seq == @seq && type == ICMP_ECHOREPLY
> +               if pid == @pid && rcv_seq == seq && type == ICMP_ECHOREPLY
>                    bool = true
>                 end
>              }
> 
>