From 5131ccf74f415807b065c85e3184e49c232a83d9 Mon Sep 17 00:00:00 2001 From: Yasuhito Takamiya Date: Wed, 8 Jul 2015 11:55:00 +0900 Subject: [PATCH] Refactor Features messages. --- Rakefile | 2 +- features/open_flow10/features_request.feature | 14 ----- features/open_flow13/features_request.feature | 14 ----- lib/pio/open_flow/echo.rb | 14 +---- lib/pio/open_flow/message.rb | 51 ++++++++++++++++--- lib/pio/open_flow/open_flow_header.rb | 14 ----- lib/pio/open_flow10/features.rb | 30 +++++------ lib/pio/open_flow10/flow_mod.rb | 2 +- lib/pio/open_flow10/hello.rb | 13 +---- lib/pio/open_flow10/packet_in.rb | 2 +- lib/pio/open_flow10/packet_out.rb | 2 +- lib/pio/open_flow10/port_status.rb | 2 +- lib/pio/open_flow13/features_reply.rb | 12 ++--- lib/pio/open_flow13/features_request.rb | 11 ++-- lib/pio/open_flow13/flow_mod.rb | 2 +- lib/pio/open_flow13/hello.rb | 5 +- lib/pio/open_flow13/packet_in.rb | 2 +- lib/pio/open_flow13/packet_out.rb | 2 +- spec/pio/open_flow10/features_request_spec.rb | 12 +++++ spec/pio/open_flow13/features_request_spec.rb | 12 +++++ 20 files changed, 102 insertions(+), 116 deletions(-) create mode 100644 spec/pio/open_flow10/features_request_spec.rb create mode 100644 spec/pio/open_flow13/features_request_spec.rb diff --git a/Rakefile b/Rakefile index 86761b78..244cc95c 100644 --- a/Rakefile +++ b/Rakefile @@ -1,7 +1,7 @@ require 'bundler/gem_tasks' RELISH_PROJECT = 'trema/pio' -FLAY_THRESHOLD = 1467 +FLAY_THRESHOLD = 1030 task default: :travis task test: [:spec, :cucumber] diff --git a/features/open_flow10/features_request.feature b/features/open_flow10/features_request.feature index 22dd9583..a244f5df 100644 --- a/features/open_flow10/features_request.feature +++ b/features/open_flow10/features_request.feature @@ -47,20 +47,6 @@ Feature: Pio::Features::Request | xid | 123 | | user_data | | - Scenario: new(xid: -1) and error - When I try to create an OpenFlow message with: - """ - Pio::Features::Request.new(xid: -1) - """ - Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer." - - Scenario: new(xid: 2**32) and error - When I try to create an OpenFlow message with: - """ - Pio::Features::Request.new(xid: 2**32) - """ - Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer." - Scenario: read When I try to parse a file named "open_flow10/features_request.raw" with "Pio::Features::Request" class Then it should finish successfully diff --git a/features/open_flow13/features_request.feature b/features/open_flow13/features_request.feature index 69a4eb74..3812c372 100644 --- a/features/open_flow13/features_request.feature +++ b/features/open_flow13/features_request.feature @@ -50,20 +50,6 @@ Feature: Pio::Features::Request | xid | 123 | | body | | - Scenario: new(xid: -1) and error - When I try to create an OpenFlow message with: - """ - Pio::Features::Request.new(xid: -1) - """ - Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer." - - Scenario: new(xid: 2**32) and error - When I try to create an OpenFlow message with: - """ - Pio::Features::Request.new(xid: 2**32) - """ - Then it should fail with "ArgumentError", "Transaction ID should be an unsigned 32-bit integer." - Scenario: new(unknown_attr: 'foo') and error When I try to create an OpenFlow message with: """ diff --git a/lib/pio/open_flow/echo.rb b/lib/pio/open_flow/echo.rb index bfd67668..08e42e00 100644 --- a/lib/pio/open_flow/echo.rb +++ b/lib/pio/open_flow/echo.rb @@ -36,18 +36,8 @@ def self.message_type(message_type) end end - user_option :transaction_id - user_option :xid - user_option :body - user_option :user_data - - def initialize(user_options = {}) - validate_user_options user_options - header_options = OpenFlowHeader::Options.parse(user_options) - body_options = user_options[:body] || user_options[:user_data] || '' - @format = self.class.const_get(:Format).new(header: header_options, - body: body_options) - end + body_option :body + body_option :user_data end end end diff --git a/lib/pio/open_flow/message.rb b/lib/pio/open_flow/message.rb index af466ca3..ae0305e5 100644 --- a/lib/pio/open_flow/message.rb +++ b/lib/pio/open_flow/message.rb @@ -19,12 +19,20 @@ def self.read(raw_data) end # rubocop:enable MethodLength - def self.user_option(name) - unless class_variable_defined?(:@@valid_options) - class_variable_set(:@@valid_options, []) + def self.body_option(name) + unless class_variable_defined?(:@@valid_body_options) + class_variable_set(:@@valid_body_options, []) end - class_variable_set(:@@valid_options, - class_variable_get(:@@valid_options) + [name]) + class_variable_set(:@@valid_body_options, + class_variable_get(:@@valid_body_options) + [name]) + end + + def initialize(user_options = {}) + validate_user_options user_options + @format = + self.class.const_get(:Format). + new(header: parse_header_options(user_options), + body: parse_body_options(user_options)) end def method_missing(method, *args, &block) @@ -34,13 +42,40 @@ def method_missing(method, *args, &block) private def validate_user_options(user_options) - unknown_options = user_options.keys - valid_options + unknown_options = + user_options.keys - valid_header_options - valid_body_options return if unknown_options.empty? fail "Unknown option: #{unknown_options.first}" end - def valid_options - self.class.class_variable_get(:@@valid_options) + def parse_header_options(user_options) + transaction_id = + user_options[:transaction_id] || user_options[:xid] || 0 + { transaction_id: transaction_id } + end + + def parse_body_options(user_options) + if valid_body_options.include?(:body) + return user_options[:body] || user_options[:user_data] || '' + end + options = user_options.dup + options.delete :transaction_id + options.delete :xid + dpid = options[:dpid] + options[:datapath_id] = dpid if dpid + options + end + + def valid_header_options + [:transaction_id, :xid] + end + + def valid_body_options + if self.class.class_variable_defined?(:@@valid_body_options) + self.class.class_variable_get(:@@valid_body_options) + else + [] + end end end end diff --git a/lib/pio/open_flow/open_flow_header.rb b/lib/pio/open_flow/open_flow_header.rb index 7ed88eec..8a1ef399 100644 --- a/lib/pio/open_flow/open_flow_header.rb +++ b/lib/pio/open_flow/open_flow_header.rb @@ -22,19 +22,5 @@ class OpenFlowHeader < BinData::Record virtual assert: -> { message_type == message_type_value } uint16 :message_length, initial_value: -> { 8 + body.length } transaction_id :transaction_id, initial_value: 0 - - # parse header options - class Options - def self.parse(options) - xid = if options.respond_to?(:to_i) - options.to_i - elsif options.respond_to?(:fetch) - options[:transaction_id] || options[:xid] || 0 - else - fail TypeError - end - { transaction_id: xid } - end - end end end diff --git a/lib/pio/open_flow10/features.rb b/lib/pio/open_flow10/features.rb index 834639c5..4e693453 100644 --- a/lib/pio/open_flow10/features.rb +++ b/lib/pio/open_flow10/features.rb @@ -10,7 +10,7 @@ class Format < BinData::Record extend OpenFlow::Format header version: 1, message_type: OpenFlow::FEATURES_REQUEST - string :body + string :body, value: '' def user_data body @@ -18,9 +18,8 @@ def user_data end def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) - body_options = user_options[:body] || user_options[:user_data] || '' - @format = Format.new(header: header_options, body: body_options) + validate_user_options user_options + @format = Format.new(header: parse_header_options(user_options)) end end @@ -101,21 +100,20 @@ def physical_ports end end - # rubocop:disable MethodLength + body_option :dpid + body_option :datapath_id + body_option :n_buffers + body_option :n_tables + body_option :capabilities + body_option :actions + body_option :ports + def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) - body_options = if user_options.respond_to?(:fetch) - user_options.delete :transaction_id - user_options.delete :xid - dpid = user_options[:dpid] - user_options[:datapath_id] = dpid if dpid - user_options - else - '' - end + validate_user_options user_options + header_options = parse_header_options(user_options) + body_options = parse_body_options(user_options) @format = Format.new(header: header_options, body: body_options) end - # rubocop:enable MethodLength end end end diff --git a/lib/pio/open_flow10/flow_mod.rb b/lib/pio/open_flow10/flow_mod.rb index 09ac66c4..7aa27aa5 100644 --- a/lib/pio/open_flow10/flow_mod.rb +++ b/lib/pio/open_flow10/flow_mod.rb @@ -84,7 +84,7 @@ class Format < BinData::Record # rubocop:disable MethodLength def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) + header_options = parse_header_options(user_options) body_options = if user_options.respond_to?(:fetch) user_options.delete :transaction_id user_options.delete :xid diff --git a/lib/pio/open_flow10/hello.rb b/lib/pio/open_flow10/hello.rb index 5f849096..4a29c210 100644 --- a/lib/pio/open_flow10/hello.rb +++ b/lib/pio/open_flow10/hello.rb @@ -16,16 +16,7 @@ def user_data end end - user_option :transaction_id - user_option :xid - user_option :body - user_option :user_data - - def initialize(user_options = {}) - validate_user_options user_options - header_options = OpenFlowHeader::Options.parse(user_options) - body_options = user_options[:body] || user_options[:user_data] || '' - @format = Format.new(header: header_options, body: body_options) - end + body_option :body + body_option :user_data end end diff --git a/lib/pio/open_flow10/packet_in.rb b/lib/pio/open_flow10/packet_in.rb index 769f5fe4..d219effe 100644 --- a/lib/pio/open_flow10/packet_in.rb +++ b/lib/pio/open_flow10/packet_in.rb @@ -71,7 +71,7 @@ class Format < BinData::Record # rubocop:disable MethodLength def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) + header_options = parse_header_options(user_options) body_options = if user_options.respond_to?(:fetch) user_options.delete :transaction_id user_options.delete :xid diff --git a/lib/pio/open_flow10/packet_out.rb b/lib/pio/open_flow10/packet_out.rb index c4198313..69590dbb 100644 --- a/lib/pio/open_flow10/packet_out.rb +++ b/lib/pio/open_flow10/packet_out.rb @@ -33,7 +33,7 @@ class Format < BinData::Record # rubocop:disable MethodLength def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) + header_options = parse_header_options(user_options) body_options = if user_options.respond_to?(:fetch) user_options.delete :transaction_id user_options.delete :xid diff --git a/lib/pio/open_flow10/port_status.rb b/lib/pio/open_flow10/port_status.rb index 3b345040..bb7eb92b 100644 --- a/lib/pio/open_flow10/port_status.rb +++ b/lib/pio/open_flow10/port_status.rb @@ -47,7 +47,7 @@ def desc # rubocop:disable MethodLength def initialize(user_options = {}) - header_options = OpenFlowHeader::Options.parse(user_options) + header_options = parse_header_options(user_options) body_options = if user_options.respond_to?(:fetch) user_options.delete :transaction_id user_options.delete :xid diff --git a/lib/pio/open_flow13/features_reply.rb b/lib/pio/open_flow13/features_reply.rb index e4da5d37..115a9fc4 100644 --- a/lib/pio/open_flow13/features_reply.rb +++ b/lib/pio/open_flow13/features_reply.rb @@ -51,13 +51,11 @@ def dpid end end - def initialize(user_attrs = {}) - header_options = OpenFlowHeader::Options.parse(user_attrs) - body_options = user_attrs.dup - body_options[:datapath_id] = - body_options[:dpid] || body_options[:datapath_id] - @format = Format.new(header: header_options, body: body_options) - end + body_option :dpid + body_option :datapath_id + body_option :n_buffers + body_option :n_tables + body_option :capabilities end end end diff --git a/lib/pio/open_flow13/features_request.rb b/lib/pio/open_flow13/features_request.rb index 2ee0f358..cbd55554 100644 --- a/lib/pio/open_flow13/features_request.rb +++ b/lib/pio/open_flow13/features_request.rb @@ -4,7 +4,7 @@ module Pio # OpenFlow 1.3 Features Request and Reply message. class Features - remove_const :Request + remove_const(:Request) if const_defined?(:Request) # OpenFlow 1.3 Features Request message. class Request < OpenFlow::Message @@ -14,15 +14,10 @@ class Format < BinData::Record header version: 4, message_type: 5 string :body, value: '' - end - def initialize(user_attrs = {}) - unknown_attrs = user_attrs.keys - [:transaction_id, :xid] - unless unknown_attrs.empty? - fail "Unknown option: #{unknown_attrs.first}" + def user_data + body end - header_options = OpenFlowHeader::Options.parse(user_attrs) - @format = Format.new(header: header_options) end end end diff --git a/lib/pio/open_flow13/flow_mod.rb b/lib/pio/open_flow13/flow_mod.rb index ec217b2f..74e05f33 100644 --- a/lib/pio/open_flow13/flow_mod.rb +++ b/lib/pio/open_flow13/flow_mod.rb @@ -146,7 +146,7 @@ class Format < BinData::Record end def initialize(user_attrs = {}) - header_attrs = OpenFlowHeader::Options.parse(user_attrs) + header_attrs = parse_header_options(user_attrs) body_attrs = { table_id: user_attrs[:table_id], match: user_attrs[:match], priority: user_attrs[:priority], diff --git a/lib/pio/open_flow13/hello.rb b/lib/pio/open_flow13/hello.rb index 63360192..ce3c3cb5 100644 --- a/lib/pio/open_flow13/hello.rb +++ b/lib/pio/open_flow13/hello.rb @@ -70,12 +70,9 @@ def version_bitmap end end - user_option :transaction_id - user_option :xid - def initialize(user_options = {}) validate_user_options user_options - header_options = OpenFlowHeader::Options.parse(user_options) + header_options = parse_header_options(user_options) body_options = { elements: [{ element_type: VERSION_BITMAP, element_length: 8, element_value: 0b10000 }] } diff --git a/lib/pio/open_flow13/packet_in.rb b/lib/pio/open_flow13/packet_in.rb index d85a8736..0d7f630b 100644 --- a/lib/pio/open_flow13/packet_in.rb +++ b/lib/pio/open_flow13/packet_in.rb @@ -68,7 +68,7 @@ def method_missing(method, *args) end def initialize(user_attrs = {}) - header_attrs = OpenFlowHeader::Options.parse(user_attrs) + header_attrs = parse_header_options(user_attrs) body_attrs = { raw_data: user_attrs[:raw_data] } @format = Format.new(header: header_attrs, body: body_attrs) end diff --git a/lib/pio/open_flow13/packet_out.rb b/lib/pio/open_flow13/packet_out.rb index dee3e12b..e8a47ebc 100644 --- a/lib/pio/open_flow13/packet_out.rb +++ b/lib/pio/open_flow13/packet_out.rb @@ -57,7 +57,7 @@ class Format < BinData::Record end def initialize(user_attrs = {}) - header_attrs = OpenFlowHeader::Options.parse(user_attrs) + header_attrs = parse_header_options(user_attrs) body_attrs = { actions: user_attrs[:actions], raw_data: user_attrs[:raw_data] } @format = Format.new(header: header_attrs, body: body_attrs) diff --git a/spec/pio/open_flow10/features_request_spec.rb b/spec/pio/open_flow10/features_request_spec.rb new file mode 100644 index 00000000..57454f07 --- /dev/null +++ b/spec/pio/open_flow10/features_request_spec.rb @@ -0,0 +1,12 @@ +require 'pio/open_flow10/features' + +describe Pio::Features::Request do + describe '.new' do + it_should_behave_like('an OpenFlow message', Pio::Features::Request) + + context "with body: 'abcde'" do + When(:message) { Pio::Features::Request.new(body: 'abcde') } + Then { message == Failure(RuntimeError, 'Unknown option: body') } + end + end +end diff --git a/spec/pio/open_flow13/features_request_spec.rb b/spec/pio/open_flow13/features_request_spec.rb new file mode 100644 index 00000000..c29ae351 --- /dev/null +++ b/spec/pio/open_flow13/features_request_spec.rb @@ -0,0 +1,12 @@ +require 'pio/open_flow13/features_request' + +describe Pio::Features::Request do + describe '.new' do + it_should_behave_like('an OpenFlow message', Pio::Features::Request) + + context "with body: 'abcde'" do + When(:message) { Pio::Features::Request.new(body: 'abcde') } + Then { message == Failure(RuntimeError, 'Unknown option: body') } + end + end +end