Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix Socket#socketpair on X19. #2014

Merged
merged 4 commits into from Nov 17, 2012

Conversation

Projects
None yet
2 participants
Contributor

frodsan commented Nov 14, 2012

Socket#socketpair type argument can accept
a symbol that references to a Socket::SOCK_*
constant.

require 'socket'

# Before

Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
# => [#<Socket:fd 11>, #<Socket:fd 12>]

Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
# => TypeError: Tried to use non-reference value 0x15a86 as type Bignum (10)

# After

Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
# => [#<Socket:fd 7>, #<Socket:fd 8>]
Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
# => [#<Socket:fd 9>, #<Socket:fd 10>]

Fixes #2011.

Contributor

frodsan commented Nov 14, 2012

Obviously, this is not finished. I'm new to Rubinius and I'm starting to use it in development. I read the contributing guide and I know that I have to add specs for this (I'll investigate how to do it). If someone has a better fix, please send it!

Anyway, I just want it to give it a try :)

Francesco Rodriguez added some commits Nov 14, 2012

Francesco Rodriguez Fix Socket#socketpair on X19.
Socket#socketpair `type` argument can accept
a symbol that references to a `Socket::SOCK_*`
constant.

    require 'socket'

    # Before

    Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
    # => [#<Socket:fd 11>, #<Socket:fd 12>]

    Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
    # => TypeError: Tried to use non-reference value 0x15a86 as type Bignum (10)

    # After

    Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
    # => [#<Socket:fd 7>, #<Socket:fd 8>]
    Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
    # => [#<Socket:fd 9>, #<Socket:fd 10>]

Fixes #2011.
66c4dca
Francesco Rodriguez Add specs for Socket#socketpair on X19 b66bb35
Contributor

frodsan commented Nov 14, 2012

I added some specs. Any feedback will be welcome :)

frodsan$ mspec spec/ruby/library/socket/socket/pair_spec.rb 
rubinius 2.0.0rc1 (1.9.3 2242f14b 2012-11-02 JI) [x86_64-apple-darwin12.2.0]
...

Finished in 0.027147 seconds

1 file, 3 examples, 7 expectations, 0 failures, 0 errors
Owner

dbussink commented Nov 14, 2012

Did you also verify the behavior in 1.8 mode?

Contributor

frodsan commented Nov 14, 2012

This is the behaviour in Ruby 1.8.7.

>> RUBY_VERSION
=> "1.8.7"
>> require 'socket'
=> true
>> Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
=> [#<Socket:0x109bfc750>, #<Socket:0x109bfc778>]
>> Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
Errno::EPROTONOSUPPORT: Protocol not supported - socketpair(2)
    from (irb):3:in `socketpair'
    from (irb):3

and this is with Rubinius -X18 mode:

>> require 'socket'
=> true
>> Socket.socketpair(Socket::PF_UNIX, Socket::SOCK_DGRAM, 0)
=> [#<Socket:0x1714>, #<Socket:0x1718>]
>> Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
TypeError: Tried to use non-reference value 0xf26e as type Bignum (10)

What should I do? Thanks :)

Owner

dbussink commented Nov 14, 2012

Probably needs a similar check to see if it's a valid constant in 1.8 mode and throw a Errno::EPROTONOSUPPORT when it isn't one.

Contributor

frodsan commented Nov 14, 2012

Something like this?

--- a/lib/18/socket.rb
+++ b/lib/18/socket.rb
@@ -682,6 +682,8 @@ class Socket < BasicSocket
       else
         raise SocketError, "unknown socket type #{type}"
       end
+    elsif !type.kind_of? Integer
+      raise SocketError, "unknown socket type #{type}"
     end

     FFI::MemoryPointer.new :int, 2 do |mp|

Result:

>> Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
SocketError: unknown socket type DGRAM
Owner

dbussink commented Nov 14, 2012

Well, we should raise the same exception as MRI. And we should also add a spec for that then

@dbussink dbussink and 1 other commented on an outdated diff Nov 15, 2012

lib/18/socket.rb
@@ -682,6 +682,8 @@ def self.socketpair(domain, type, protocol, klass=self)
else
raise SocketError, "unknown socket type #{type}"
end
+ elsif type.kind_of? Symbol
+ raise Errno::EPROTONOSUPPORT.new("socketpair(2)")
@dbussink

dbussink Nov 15, 2012

Owner

We usually use the style raise Errno::EPROTONOSUPPORT, "socketpair(2)". Another thing is that it's easy to make it more descriptive by including the name.

Also probably best to use !type.kind_of? Integer, because other objects also don't work and could you add a spec for 1.8 mode too?

@frodsan

frodsan Nov 15, 2012

Contributor

Ok, I'll update this. Thanks :)

@dbussink

dbussink Nov 15, 2012

Owner

BTW, we deliberately don't have Symbol#to_i, it's an old relic from the past

@dbussink

dbussink Nov 15, 2012

Owner

Something like this works fine for me here: https://gist.github.com/5f99a7daa7dd172761f0

@frodsan frodsan commented on an outdated diff Nov 15, 2012

lib/18/socket.rb
@@ -682,6 +682,8 @@ def self.socketpair(domain, type, protocol, klass=self)
else
raise SocketError, "unknown socket type #{type}"
end
+ elsif type.kind_of? Symbol
@frodsan

frodsan Nov 15, 2012

Contributor

@dbussink I think this is not an easy fix:

>> RUBY_VERSION
=> "1.8.7"
>> require 'socket'
=> true
>> :DGRAM.to_i
=> 16501
>> Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
Errno::EPROTONOSUPPORT: Protocol not supported - socketpair(2)
    from (irb):11:in `socketpair'
    from (irb):11
>> Socket.socketpair(Socket::PF_UNIX, 16501, 0)
Errno::EPROTONOSUPPORT: Protocol not supported - socketpair(2)
    from (irb):12:in `socketpair'
    from (irb):12

Rubinius X18 mode:

>> require 'socket'
=> true
>> :DGRAM.to_i
NoMethodError: undefined method `to_i' on DGRAM:Symbol.
>> Socket.socketpair(Socket::PF_UNIX, :DGRAM, 0)
TypeError: Tried to use non-reference value 0xe70e as type Bignum (10)
>> Socket.socketpair(Socket::PF_UNIX, 16501, 0)
=> [#<Socket:0x1344>, #<Socket:0x1348>]

@frodsan frodsan commented on the diff Nov 15, 2012

lib/18/socket.rb
@@ -682,6 +682,8 @@ def self.socketpair(domain, type, protocol, klass=self)
else
raise SocketError, "unknown socket type #{type}"
end
+ elsif !type.kind_of? Integer
+ raise Errno::EPROTONOSUPPORT, type.inspect
@frodsan

frodsan Nov 15, 2012

Contributor

@dbussink updated. I'm having a problem when running the tests. Do you know how to run mspec in 1.8 mode? Thanks.

$ bin/mspec spec/ruby/library/socket/socket/socketpair_spec.rb 
rubinius 2.0.0rc1 (1.9.3 b66bb353 2012-11-02 JI) [x86_64-apple-darwin12.2.0]
...F

1)
Socket#socketpair raises Errno::EPROTONOSUPPORT if socket type is not a String or Integer FAILED
Expected Errno::EPROTONOSUPPORT but no exception was raised
           { } in Object#__script__ at spec/ruby/library/socket/shared/socketpair.rb:26
@dbussink

dbussink Nov 15, 2012

Owner

You can set RBXOPT. Our default is still 1.8 mode, so normally that just runs. You can then specify 1.9 mode with -tx19 passed to mspec. We should probably also add -tx18.

In this case you also need to not run the spec in 1.8 mode, so you need a version guard like ruby_version_is ""..."1.9" do which means run up to 1.9 exclusive.

Contributor

frodsan commented Nov 15, 2012

@dbussink Updated. I can't run the tests in X18 mode yet :(

$ RBXOPT=-X18 bin/mspec spec/ruby/library/socket/socket/socketpair_spec.rb
rubinius 2.0.0rc1 (1.9.3 6b18a0e4 2012-11-02 JI) [x86_64-apple-darwin12.2.0]
...

Finished in 0.018068 seconds

1 file, 3 examples, 7 expectations, 0 failures, 0 errors
Contributor

frodsan commented Nov 16, 2012

Oops ... I misinterpreted the documentation:

./configure --enable-version=1.9,2.0 --default-version=1.9

I didn't enable 1.8 mode. Now, works fine. Thanks 👍

$ RBXOPT=-X18 bin/mspec spec/ruby/library/socket/socket/pair_spec.rb 
rubinius 2.0.0rc1 (1.8.7 927ff0e8 2012-11-02 JI) [x86_64-apple-darwin12.2.0]
..

Finished in 0.022539 seconds

1 file, 2 examples, 2 expectations, 0 failures, 0 errors
Owner

dbussink commented Nov 16, 2012

I strongly suggest not changing configure options when developing on Rubinius, to prevent issues such as these. Also could you please split the changes for 1.8 mode in a separate commit for the spec addition? Just like the changes for 1.9 mode.

Contributor

frodsan commented Nov 16, 2012

Done.

@dbussink dbussink added a commit that referenced this pull request Nov 17, 2012

@dbussink dbussink Merge pull request #2014 from frodsan/fix_socket_constants
Fix Socket#socketpair on X19.
bcb15bd

@dbussink dbussink merged commit bcb15bd into rubinius:master Nov 17, 2012

1 check failed

default The Travis build failed
Details
Owner

dbussink commented Nov 17, 2012

Thnx!

@ghost

ghost commented Nov 17, 2012

@frodsan Thanks :-)

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