Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The server handler should only be able to craft a response packet and should not receive a net.PacketConn #190

Open
begetan opened this issue Nov 7, 2018 · 6 comments

Comments

@begetan
Copy link

begetan commented Nov 7, 2018

I've wanted to rewrite my example DHCP server with recently added server.go implementation but haven't found no library code for sending packet back to network.

On the example code sending function was provided in user handler code, but actually this function is more complicated and should have a deal with different OSes, socket options and should be outside of user code in library implementation. So proposal is to make handler work only with packet manipulation, not network binding.

-- type Handler func(conn net.PacketConn, peer net.Addr, m *DHCPv4)

++ type Handler func(peer net.Addr, request *DHCPv4)(*DHCPv4, error) {

    r, err := dhcpv4.NewReplyFromRequest(request)
    msgType := request.MessageType()
    switch msgType {
        case dhcpv4.MessageTypeDiscover:
    ...
    }
-- if _, err := conn.WriteTo(r.ToBytes(), peer); err != nil {
-- log.Printf("Cannot reply to client: %v", err)
-- }
++ return r, err    
}

Rest part of initial proposal is omitted for now

@pmazzini
Copy link
Collaborator

pmazzini commented Nov 8, 2018

Hi @begetan,

I wanted to rewrite my example DHCP server with recently added server.go implementation but found no code for sending packet back to network.

Have a look at server_test.go, you can send packets back to the network using the conn net.PacketConn. We definitely need to improve the examples/documentation, which is being tracked in #188.

@insomniacslk
Copy link
Owner

Basically what @pmazzini said. I will write examples soon (days hopefully).
Handling errors and messages at the moment can be done implicitly with channels, but I agree that it would be nice to have them explicitly from the handler. I will have a look at that too

@insomniacslk
Copy link
Owner

Because of the above, @begetan could you please rename this issue to reflect what has to be done, or close this and reopen a new one? Thanks!

@begetan begetan changed the title New server.go implementation only reads packets Add posibility to handle user defined structure in server.go implementation Nov 8, 2018
@begetan
Copy link
Author

begetan commented Nov 10, 2018

@pmazzini
I've checked server_test.go and understood you way of handling replies.
Since I am coming to this library from other project I just trying to adopt my code based on current knowledge.

In my point of view there are some quite reasonable abstraction levels for public API:

  1. DHCP bytes handling - basic functions for protocol bytes manipulation
  2. DHCP messages packet handling (client/server) - basic functions for creating and investigation of real packets
  3. Network support (client/server) - helpers to provides Network/OS specific logic
  4. Server/Client logic - at this level users write their server or client implementation

There is one already open discussion about implementation of 1 and 2 level in library: #186

But here I would like to open another discussion about implimentation of layers 3 and 4.
In my humble opinion it is expectable to have one level functions in the same level of abstraction in code, like conn.ReadFrom and conn.WriteTo. According this logic user handler should get packet for processing and return back new packet to the library.

I just showed an example of separation logic between layer 3 and 4 including Interface method to handle server level configuration. But let's focus on architecture first because it's most important for quite new library. I believe your code implementation is much more professional than other projects and you want to share experience for wide range of programmers. So it should be structured well.

@pmazzini
Copy link
Collaborator

@begetan Do you mean that the server handler should only be able to craft a response packet and should not receive a net.PacketConn? If that is the case then I kind of agree.

Can the issue title be properly updated?

@begetan
Copy link
Author

begetan commented Jan 29, 2019

@pmazzini

After better thinking and evaluation of current implementation I see no reason to pass global server parameters to user handler. It depends on concurency model and user architecture. But now I am definitely sure about handling only packet structure, not connections.

@begetan begetan changed the title Add posibility to handle user defined structure in server.go implementation The server handler should only be able to craft a response packet and should not receive a net.PacketConn Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants