inch をかけると Pio::Dhcp::DhcpTlvOptions#dhcp_server_identifier_

確認したところ、ご指摘頂いた箇所以外に、メソッド名に型名を付けているものは見当たりませんでした。相談ですが、この<code class="notrans

補足ですが、現時点での実装では、下記は、必要ありません。 <p

Delete `_hash' from *_hash methods about pio HOT 24 CLOSED

yasuhito commented on September 14, 2024

Delete `_hash' from *_hash methods

from pio.

Comments (24)

shun159 commented on September 14, 2024

ご指摘有難う御座います。
こちら確認したら、修正致します。

from pio.

shun159 commented on September 14, 2024

メソッド名、後程相談させてください。
宜しくお願いします。

from pio.

shun159 commented on September 14, 2024

確認したところ、ご指摘頂いた箇所以外に、
メソッド名に型名を付けているものは見当たりませんでした。
相談ですが、この*_hashについては、これがHashでなくなったとしても、
TLV構造を意識しなければならないはずですので、
そのため、ここはあえて、*_tlvというメソッド名にすべきと考えます。

変更点は下記のPRの通りです。
#60

ご意見の程、宜しくお願い致します。

from pio.

yasuhito commented on September 14, 2024

相談ですが、この__hashについては、これがHashでなくなったとしても、
TLV構造を意識しなければならないはずですので、
そのため、ここはあえて、__tlvというメソッド名にすべきと考えます。

指摘したのは Hash とかの Ruby クラス名がメソッド名に入ってるとダメ、という点でした。

ねんのため詳しく説明しますと、

members_array = ['eishun', 'yasuhito']  # _array が冗長。
members = ['eishun', 'yasuhito'] # こうすべき

さらに members のデータ構造を変えたときに、_array を書き直すのを忘れると悲惨です。

# いつの間にか Hash になってた。バグる元。
members_array = { :eishun => 'Eishun Kondoh',
                               :yasuhito => 'Yasuhito Takamiya' }

これとは別に、_tlv をつけるかどうかはいまコードを読んでるので少しおまちください。

from pio.

yasuhito commented on September 14, 2024

_tlv をつけるかどうかですが、プレフィクスの Pio::Dhcp::DhcpTlvOptions に TLV と入ってるのでいらない気がします。

でその前にそもそも、この DhcpTlvOptions にすべての TLV のオプション? がいっしょくたに入ってるのが元凶な気がします。LLDP のコードみたいに TLV ごとにクラスを分けたほうが分かりやすくないでしょうか?

from pio.

shun159 commented on September 14, 2024

確認致します。少々お待ちください。

from pio.

shun159 commented on September 14, 2024

承知しました。

修正幅多そうなので、
まず、方針を先に決めておきたいと思います。
後程、相談させて頂ければと存じます。

宜しくお願い致します。

from pio.

yasuhito commented on September 14, 2024

とりあえずコードを簡単にしようと思い、Dhcp::Discover をバラしてみました。継承をなくして、なるべくベタに書こうと書き直し中のがこれ↓です。

require 'forwardable'
require 'pio/dhcp/boot_request_options'

module Pio
  class Dhcp
    # Dhcp Discover packet parser and generator
    class Discover
      TYPE = 1

      extend Forwardable

      def_delegators :@frame, :destination_mac
      def_delegators :@frame, :source_mac
      def_delegators :@frame, :ether_type
      def_delegators :@frame, :ip_version
      def_delegators :@frame, :ip_header_length
      def_delegators :@frame, :ip_type_of_service
      def_delegators :@frame, :ip_total_length
      def_delegators :@frame, :ip_identifier
      def_delegators :@frame, :ip_flag
      def_delegators :@frame, :ip_fragment
      def_delegators :@frame, :ip_ttl
      def_delegators :@frame, :ip_protocol
      def_delegators :@frame, :ip_header_checksum
      def_delegators :@frame, :ip_source_address
      def_delegators :@frame, :ip_destination_address
      def_delegators :@frame, :udp_src_port
      def_delegators :@frame, :udp_dst_port
      def_delegators :@frame, :udp_length
      def_delegators :@frame, :udp_checksum
      def_delegators :@frame, :dhcp
      def_delegators :@frame, :optioal_tlvs
      def_delegators :@frame, :boot_message_type
      def_delegators :@frame, :message_type
      def_delegators :@frame, :hw_addr_type
      def_delegators :@frame, :hw_addr_len
      def_delegators :@frame, :hops
      def_delegators :@frame, :transaction_id
      def_delegators :@frame, :seconds
      def_delegators :@frame, :bootp_flags
      def_delegators :@frame, :client_ip_address
      def_delegators :@frame, :your_ip_address
      def_delegators :@frame, :next_server_ip_address
      def_delegators :@frame, :relay_agent_ip_address
      def_delegators :@frame, :client_mac_address
      def_delegators :@frame, :server_host_name
      def_delegators :@frame, :boot_file_name
      def_delegators :@frame, :server_identifier
      def_delegators :@frame, :renewal_time_value
      def_delegators :@frame, :rebinding_time_value
      def_delegators :@frame, :ip_address_lease_time
      def_delegators :@frame, :subnet_mask
      def_delegators :@frame, :client_identifier
      def_delegators :@frame, :requested_ip_address
      def_delegators :@frame, :parameters_list
      def_delegators :@frame, :to_binary

      def self.create_from(frame)
        message = allocate
        message.instance_variable_set :@frame, frame
        message
      end

      def initialize(user_options)
        type_merged_options = user_options.merge(type: 1)
        options = BootRequestOptions.new(type_merged_options)
        @frame = Dhcp::Frame.new(options.to_hash)
      end
    end
  end
end

質問です:

def_delegators で定義してるメソッドで、DHCP_DISCOVER にいらないものはありますか?
BootRequestOptions はすべての DHCP フレームに共通に読めましたが、これに渡せるオプションで DHCP_DISCOVER にいらないものはありますか?

from pio.

shun159 commented on September 14, 2024

有難う御座います。(私も、案を出すために、少し書いてた最中でした…)

def_delegators で定義してるメソッドで、DHCP_DISCOVER にいらないものはありますか?

現時点での実装では、
下記は、必要ありません。

      def_delegators :@frame, :server_identifier
      def_delegators :@frame, :renewal_time_value
      def_delegators :@frame, :rebinding_time_value
      def_delegators :@frame, :ip_address_lease_time
      def_delegators :@frame, :subnet_mask

BootRequestOptions はすべての DHCP フレームに共通に読めましたが、これに渡せるオプションでDHCP_DISCOVER にいらないものはありますか?

現時点での実装では、
下記は、必要ありません。

      option :destination_mac
      option :server_identifier

宜しくお願い致します。

from pio.

shun159 commented on September 14, 2024

補足ですが、

現時点での実装では、
下記は、必要ありません。

と言っているのは、RFCでは”任意”となっている箇所のうちで、
あまり使用されないフィールドを省略して、短くフレームを生成できるように
しているためです。

宜しくお願い致します。

from pio.

yasuhito commented on September 14, 2024

今後の方針として、最初はベタに書いて後でリファクタリングするようにしましょう。

まずは、Dhcp::Discover::Options からです。

必須のオプションとそうでないもの、現時点で実装していないものの区別をしたいと思います。

lib/pio/arp/reply.rb と spec/pio/arp/reply/options_spec を参考に、

テスト: spec/pio/dhcp/discover/options_spec.rb
コード: lib/pio/dhcp/discover/options.rb

を書いてみてください。

そのとき、

必須オプションが入ってないとエラーになること、
ふつうのオプションは省略するとデフォルト値になること、
実装してないものは RSpec の pending が上がること、
をチェックしてください。

できあがったら PR かここに貼ってもらえれば refactor_dhcp ブランチに追加していきます。

from pio.

shun159 commented on September 14, 2024

有難う御座います。
承知しました。

お手数おかけしました。

from pio.

shun159 commented on September 14, 2024

すみません、念のためですが、

LLDP のコードみたいに TLV ごとにクラスを分けたほうが分かりやすくないでしょうか?

へ修正しつつ、という感じでOKですか。

宜しくお願いします。

from pio.

yasuhito commented on September 14, 2024

#60 (comment) に書いたように、最初は DHCP_DISCOVER オプションのテストと実装の 2 ファイルだけおねがいします。一度にやるとむずかしいので。。

from pio.

shun159 commented on September 14, 2024

すみません、失礼しました。
有難う御座います。

from pio.

yasuhito commented on September 14, 2024

テスト: spec/pio/dhcp/discover/options_spec.rb

コード: lib/pio/dhcp/discover/options.rb

@shun159 これ時間かかりそうでしょうか?
もし大変そうだったら、必須オプションと普通のオプションをリストにまとめてここに貼ってもらうだけでも助かります。

from pio.

shun159 commented on September 14, 2024

いえ、おそらく手を付けられれば、
それほど時間はかかりませんが、
今日は別件での対応で、少し手が付けられそうにありません。

もし、お急ぎでしたら、リストを先におだしして、
それから、今晩～明日午前くらいで対応します。いかがでしょうか？

恐縮ですが、何卒よろしくお願いいたします。

from pio.

yasuhito commented on September 14, 2024

そこまで急ぎではないので大丈夫です。
ただ 0.4.1 はできれば早めに出しておきたいので、もし分からないこととかあったら早めに言ってください。

from pio.

shun159 commented on September 14, 2024

承知しました。すみません…毎度、有難うございます。

from pio.

shun159 commented on September 14, 2024

lib/pio/arp/reply.rb と spec/pio/arp/reply/options_spec を参考に、

テスト: spec/pio/dhcp/discover/options_spec.rb
コード: lib/pio/dhcp/discover/options.rb

を書いてみてください。

#65 にて出しました。アドバイスや変なところありましたら、ご指摘お願いします。

from pio.

shun159 commented on September 14, 2024

Field	DHCP Discover	DHCP Request
Operation code	BOOTREQUEST	BOOTREQUEST
Hardware code	(From "Assigned Numbers" RFC)	(From "Assigned Numbers" RFC)
Hardware address length	length of hardware address	length of hardware address
ttl	0	0
transaction id	'xid' from client	'xid' from server DHCPOFFER message
seconds	0 or seconds since DHCP process started	0 or seconds since DHCP process started
flags	Set 'BROADCAST'flag if client	Set 'BROADCAST'flag if client
	requires broadcast reply	requires broadcast reply
client ip address	0	0 or client's network address
your ip address	0	0
server ip address	0	0
relay agent ip address	0	0
client hardware address	client's hardware address	client's hardware address
server name	options, if indicated in 'sname/file'	options, if indicated in 'sname/file'
	option; otherwise unused	option; otherwise unused
file name	options, if indicated in 'sname/file'	options, if indicated in 'sname/file'
	option; otherwise unused	option; otherwise unused
requested ip address	may	must
lease time value	may	may
dhcp vendor extentions	may	may
dhcp message type	DHCP Discover	DHCP Request
client id	may	may
vendor class id	may	may
server id	must not	must
requested parameter list	may	may
maximum dhcp message size	may	may
message	-	-
site specific	may	may
others	may	may

from pio.

yasuhito commented on September 14, 2024

表示が崩れてるので、できれば表にしてもらえるとうれしいです。
https://help.github.com/articles/github-flavored-markdown

from pio.

shun159 commented on September 14, 2024

修正しました。

from pio.

yasuhito commented on September 14, 2024

表ありがとうございます。
#65 のほうに転記して、このチケットを閉じてもらえると助かります。

from pio.

Delete `_hash' from *_hash methods about pio HOT 24 CLOSED

Comments (24)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent