Experiment: try using FFI::Struct for iptables instead of CStruct

FFI::Struct handles nested structs and nested arrays much better, and avoids
duplicating logic about structure alignment (which it probably does more
correctly that CStruct)

However it's awkward to use in other ways. e.g. no accessor methods;
no proper #inspect; no ntohl for in_addr; no zero-sized arrays at end
of struct; no hooks to convert int32 <-> IPAddr as far as I can see.

lib/linux/iptables.rb

@@ -1,11 +1,35 @@
 require 'socket'
 require 'linux/constants'
-require 'linux/c_struct'
+require 'ffi'
-require 'linux/netlink/message'  # just for :dev_name type
+
+# Good things about FFI::Struct:
+# - robust pre-existing code
+# - good handling of nested structures and nested arrays
+# Bad things about FFI::Struct:
+# - no Hash initialization:  MyStruct.new(:foo=>1, :bar=>2)
+# - no accessor methods      m.foo = 1
+# - can't do zero size array at end of struct: layout :foo, [Foo, 0]
+# - no network-order fields? (in_addr)
+# - no decent inspect (fix this below)
+
+class FFI::Struct
+  def inspect
+    res = "#<#{self.class}"
+    members.zip(values).each do |m,v|
+      res << " #{m}=#{v.inspect}"
+    end
+    res << ">"
+  end
+end
+
+class FFI::StructLayout::CharArray
+  def inspect
+    to_s.inspect
+  end
+end
 
 module Linux
   module Ext
-    require 'ffi'
     extend FFI::Library
     ffi_lib FFI::Library::LIBC
     attach_function :getsockopt, [:int, :int, :int, :buffer_inout, :buffer_inout], :int
@@ -20,10 +44,6 @@ module Linux
   # libip4tc.c and libip6tc.c)
   #
   #    filter = Linux::Iptables4.table("filter")
-  #
-  # TODO: should we use ffi's structures instead of CStruct?
-  # We have to use ffi anyway, until ruby getsockopt supports buffer passing.
-  # http://redmine.ruby-lang.org/issues/4645
   class Iptables
     def self.inherited(subclass) #:nodoc:
       subclass.instance_variable_set(:@tables, {})
@@ -66,36 +86,39 @@ module Linux
     
     def getsockopt(level, optname, buf)
       buflen = FFI::Buffer.new :socklen_t
-      if buflen.length == 4
+      if buflen.size == 4
-        buflen.put_uint32(0, buf.length)
+        buflen.put_uint32(0, buf.size)
-      elsif buflen.length == 8
+      elsif buflen.size == 8
-        buflen.put_uint64(0, buf.length)
+        buflen.put_uint64(0, buf.size)
       else
-        raise "Unexpected buflen length: #{buflen.length}"
+        raise "Unexpected buflen length: #{buflen.size}"
       end
       res = Ext.getsockopt(@socket.fileno, level, optname, buf, buflen)
       raise "getsockopt error: #{res}" if res < 0  # FIXME: get errno?
-      buf.get_bytes(0, buflen.length == 4 ? buflen.get_uint32(0) : buflen.get_uint64(0))
+      res   # unlike Ruby's getsockopt, we return the length, not the buf
     end
     
     def reload
-      buf = FFI::Buffer.new IPTGetInfo.bytesize
+      info = IPTGetInfo.new
-      buf.put_string(0, @name)
+      info[:name] = @name
-      info = self.class::STRUCT_GETINFO.parse(getsockopt(self.class::TC_IPPROTO, self.class::SO_GET_INFO, buf))
+      getsockopt(self.class::TC_IPPROTO, self.class::SO_GET_INFO, info)
-      #warn "valid_hooks=0x%08x, num_entries=%d, size=%d" % [info.valid_hooks, info.num_entries, info.size]
+      #warn "valid_hooks=0x%08x, num_entries=%d, size=%d" % [info[:valid_hooks], info[:num_entries], info[:size]]
 
-      buf2 = FFI::Buffer.new(self.class::STRUCT_GET_ENTRIES.bytesize + info.size)
+      init = self.class::STRUCT_GET_ENTRIES.new
-      buf2.put_bytes(0, self.class::STRUCT_GET_ENTRIES.new(:name=>@name, :size=>info.size).to_str)
+      init[:name] = @name
+      init[:size] = info[:size]
+      buf2 = FFI::MemoryPointer.new(self.class::STRUCT_GET_ENTRIES_SIZE + info[:size])
+      buf2.put_bytes(0, init.pointer.get_bytes(0, self.class::STRUCT_GET_ENTRIES_SIZE))
       getsockopt(self.class::TC_IPPROTO, self.class::SO_GET_ENTRIES, buf2)
 
       res = []
-      ptr = self.class::STRUCT_GET_ENTRIES.bytesize
+      offset = self.class::STRUCT_GET_ENTRIES_SIZE
-      limit = ptr + info.size
+      limit = offset + info[:size]
-      while ptr < limit
+      while offset < limit
-        res << self.class::STRUCT_ENTRY.parse(buf2.get_bytes(ptr, self.class::STRUCT_ENTRY.bytesize))
+        res << self.class::STRUCT_ENTRY.new(buf2 + offset)
-        ptr += res.last.next_offset
+        offset += res.last[:next_offset]
       end
-      raise "Error parsing rules: got #{res.size} instead of #{info.num_entries}" if res.size != info.num_entries
+      raise "Error parsing rules: got #{res.size} instead of #{info[:num_entries]}" if res.size != info[:num_entries]
       @rules = res
     end
   end

lib/linux/iptables4.rb

@@ -1,4 +1,5 @@
 require 'linux/iptables'
+require 'ipaddr'
 
 module Linux
   #-
@@ -6,55 +7,51 @@ module Linux
   #+
 
   # struct ipt_getinfo
-  class IPTGetInfo < CStruct
+  class IPTGetInfo < FFI::Struct
-    field :name, :pattern=>"Z#{IPT_TABLE_MAXNAMELEN}", :default=>EMPTY_STRING
+    layout :name, [:char, IPT_TABLE_MAXNAMELEN],
-    field :valid_hooks, :int
+      :valid_hooks, :uint,
-    #field :hook_entry, :pattern=>"I#{NF_INET_NUMHOOKS}", :default=>[0]*NF_INET_NUMHOOKS
+      :hook_entry, [:uint, NF_INET_NUMHOOKS],
-    #field :underflow, :pattern=>"I#{NF_INET_NUMHOOKS}", :default=>[0]*NF_INET_NUMHOOKS
+      :underflow, [:uint, NF_INET_NUMHOOKS],
-    field :hook_entry, :pattern=>"a#{NF_INET_NUMHOOKS*4}", :default=>EMPTY_STRING
+      :num_entries, :uint,
-    field :underflow, :pattern=>"a#{NF_INET_NUMHOOKS*4}", :default=>EMPTY_STRING
+      :size, :uint
-    field :num_entries, :int
+  end
-    field :size, :int
+
+  class IPTIP < FFI::Struct
+    layout :src, :int32,  # FIXME: needs ntohl
+      :dst, :int32,
+      :smsk, :int32,
+      :dmsk, :int32,
+      :iniface, [:char, IFNAMSIZ],
+      :outiface, [:char, IFNAMSIZ],
+      :iniface_mask, [:uchar, IFNAMSIZ],
+      :outiface_mask, [:uchar, IFNAMSIZ],
+      :proto, :uint16,
+      :flags, :uint8,
+      :invflags, :uint8
+  end
+
+  # struct xt_counters (netfilter/x_tables.h)
+  class XTCounters < FFI::Struct
+    layout :pcnt, :uint64,
+      :bcnt, :uint64
+  end
+    
+  # struct ipt_entry
+  class IPTEntry < FFI::Struct
+    layout :ip, IPTIP,
+      :nfcache, :uint,
+      :target_offset, :uint16,	# size of ipt_entry + matches
+      :next_offset, :uint16,	# size of ipt_entry + matches + target
+      :comefrom, :uint,
+      :counters, XTCounters,
+      :elems, [:uchar, 1]	# should be [:uchar, 0]
   end
 
   # struct ipt_get_entries
-  class IPTGetEntries < CStruct
+  class IPTGetEntries < FFI::Struct
-    field :name, :pattern=>"Z#{IPT_TABLE_MAXNAMELEN}", :default=>EMPTY_STRING
+    layout :name, [:uchar, IPT_TABLE_MAXNAMELEN],
-    field :size, :uint
+      :size, :uint,
-    field :entrytable, :binary, :align=>1.size   # struct ipt_entry entrytable[0]
+      :entrytable, [IPTEntry, 1]	# should be [IPTEntry, 0]
-  end
-  
-  # struct ipt_entry
-  class IPTEntry < CStruct
-    #### struct ipt_ip
-    field :src, :nl   # struct in_addr
-    field :dst, :nl
-    field :smsk, :nl
-    field :dmsk, :nl
-    field :iniface, :dev_name
-    field :outiface, :dev_name
-    field :iniface_mask, :dev_name
-    field :outiface_mask, :dev_name
-    field :proto, :uint16
-    field :flags, :uchar
-    field :invflags, :uchar
-    ####
-    field :nfcache, :uint
-    field :target_offset, :uint16	# size of ipt_entry + matches
-    field :next_offset, :uint16		# size of ipt_entry + matches + target
-    field :comefrom, :uint
-    ### struct xt_counters
-    field :packet_count, :uint64, :align => 8
-    field :byte_count, :uint64
-    ###
-    field :elems, :binary   # matches (if any), then the target
-
-    def after_parse
-      self.src = src == 0 ? nil : IPAddr.new(src, Socket::AF_INET)
-      self.dst = dst == 0 ? nil : IPAddr.new(dst, Socket::AF_INET)
-      self.smsk = smsk == 0 ? nil : IPAddr.new(smsk, Socket::AF_INET)
-      self.dmsk = dmsk == 0 ? nil : IPAddr.new(dmsk, Socket::AF_INET)
-    end
   end
 
   # Class for handling iptables. Note that this doesn't actually use
@@ -72,6 +69,8 @@ module Linux
     STRUCT_ENTRY	= IPTEntry
     STRUCT_GETINFO	= IPTGetInfo
     STRUCT_GET_ENTRIES	= IPTGetEntries
+    # This is a frig because of [1] instead of [0] above
+    STRUCT_GET_ENTRIES_SIZE = IPTGetEntries.offset_of(:entrytable)
   end
 end