From 227b290b776ee42396097a36d3f0a93299ad782d Mon Sep 17 00:00:00 2001 From: cnderrauber Date: Mon, 10 Oct 2022 10:11:36 +0800 Subject: [PATCH] Solve comments and clean code Solve comments and clean code --- agent_udpmux_test.go | 8 +------- errors.go | 5 ++++- gather_test.go | 3 +-- tcp_mux_multi.go | 2 +- udp_mux.go | 26 ++++++++++++++------------ udp_mux_multi.go | 13 +------------ udp_mux_multi_test.go | 9 +++------ udp_mux_test.go | 4 +--- udp_mux_universal.go | 17 +++++++---------- udp_mux_universal_test.go | 3 +-- 10 files changed, 34 insertions(+), 56 deletions(-) diff --git a/agent_udpmux_test.go b/agent_udpmux_test.go index f0930f0..660984c 100644 --- a/agent_udpmux_test.go +++ b/agent_udpmux_test.go @@ -35,17 +35,11 @@ func TestMuxAgent(t *testing.T) { require.NoError(t, err) loggerFactory := logging.NewDefaultLoggerFactory() - udpMux, err := NewUDPMuxDefault(UDPMuxParams{ + udpMux := NewUDPMuxDefault(UDPMuxParams{ Logger: loggerFactory.NewLogger("ice"), UDPConn: c, }) - require.NoError(t, err) - defer func() { - _ = udpMux.Close() - _ = c.Close() - }() - muxedA, err := NewAgent(&AgentConfig{ UDPMux: udpMux, CandidateTypes: []CandidateType{CandidateTypeHost}, diff --git a/errors.go b/errors.go index 8576e26..838e714 100644 --- a/errors.go +++ b/errors.go @@ -140,6 +140,9 @@ var ( errNotImplemented = errors.New("not implemented yet") errNoUDPMuxAvailable = errors.New("no UDP mux is available") errNoTCPMuxAvailable = errors.New("no TCP mux is available") + errInvalidAddress = errors.New("invalid address") + + // UDPMuxDefault should not listen on unspecified address, but to keep backward compatibility, don't return error now. + // will be used in the future. // errListenUnspecified = errors.New("can't listen on unspecified address") - errInvalidAddress = errors.New("invalid address") ) diff --git a/gather_test.go b/gather_test.go index c689b03..ac595bd 100644 --- a/gather_test.go +++ b/gather_test.go @@ -507,8 +507,7 @@ func TestMultiUDPMuxUsage(t *testing.T) { }() expectedPorts = append(expectedPorts, port) - muxDefault, err := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn}) - assert.NoError(t, err) + muxDefault := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn}) udpMuxInstances = append(udpMuxInstances, muxDefault) idx := i defer func() { diff --git a/tcp_mux_multi.go b/tcp_mux_multi.go index 0f7d1c5..d6e7649 100644 --- a/tcp_mux_multi.go +++ b/tcp_mux_multi.go @@ -6,7 +6,7 @@ package ice import "net" // AllConnsGetter allows multiple fixed TCP ports to be used, -// each which is multiplexed like TCPMux. AllConnsGetter also acts as +// each of which is multiplexed like TCPMux. AllConnsGetter also acts as // a TCPMux, in which case it will return a single connection for one // of the ports. type AllConnsGetter interface { diff --git a/udp_mux.go b/udp_mux.go index 8b39f83..ac00626 100644 --- a/udp_mux.go +++ b/udp_mux.go @@ -52,16 +52,15 @@ type UDPMuxParams struct { } // NewUDPMuxDefault creates an implementation of UDPMux -func NewUDPMuxDefault(params UDPMuxParams) (*UDPMuxDefault, error) { +func NewUDPMuxDefault(params UDPMuxParams) *UDPMuxDefault { if params.Logger == nil { params.Logger = logging.NewDefaultLoggerFactory().NewLogger("ice") } var localAddrsForUnspecified []net.Addr if addr, ok := params.UDPConn.LocalAddr().(*net.UDPAddr); !ok { - return nil, errInvalidAddress + params.Logger.Errorf("LocalAddr is not a net.UDPAddr, got %T", params.UDPConn.LocalAddr()) } else if ok && addr.IP.IsUnspecified() { - // return nil, errListenUnspecified // For unspecified addresses, the correct behavior is to return errListenUnspecified, but // it will break the applications that are already using unspecified UDP connection // with UDPMuxDefault, so print a warn log and create a local address list for mux. @@ -75,15 +74,18 @@ func NewUDPMuxDefault(params UDPMuxParams) (*UDPMuxDefault, error) { networks = []NetworkType{NetworkTypeUDP4, NetworkTypeUDP6} default: - return nil, errInvalidAddress + params.Logger.Errorf("LocalAddr expected IPV4 or IPV6, got %T", params.UDPConn.LocalAddr()) } - muxNet := vnet.NewNet(nil) - ips, err := localInterfaces(muxNet, nil, nil, networks) - if err != nil { - return nil, err - } - for _, ip := range ips { - localAddrsForUnspecified = append(localAddrsForUnspecified, &net.UDPAddr{IP: ip, Port: addr.Port}) + if len(networks) > 0 { + muxNet := vnet.NewNet(nil) + ips, err := localInterfaces(muxNet, nil, nil, networks) + if err == nil { + for _, ip := range ips { + localAddrsForUnspecified = append(localAddrsForUnspecified, &net.UDPAddr{IP: ip, Port: addr.Port}) + } + } else { + params.Logger.Errorf("failed to get local interfaces for unspecified addr: %v", err) + } } } @@ -103,7 +105,7 @@ func NewUDPMuxDefault(params UDPMuxParams) (*UDPMuxDefault, error) { go m.connWorker() - return m, nil + return m } // LocalAddr returns the listening address of this UDPMuxDefault diff --git a/udp_mux_multi.go b/udp_mux_multi.go index f7a0bc3..ff5cdcc 100644 --- a/udp_mux_multi.go +++ b/udp_mux_multi.go @@ -111,21 +111,10 @@ func NewMultiUDPMuxFromPort(port int, opts ...UDPMuxFromPortOption) (*MultiUDPMu muxs := make([]UDPMux, 0, len(conns)) for _, conn := range conns { - mux, muxErr := NewUDPMuxDefault(UDPMuxParams{Logger: params.logger, UDPConn: conn}) - if muxErr != nil { - err = muxErr - break - } + mux := NewUDPMuxDefault(UDPMuxParams{Logger: params.logger, UDPConn: conn}) muxs = append(muxs, mux) } - if err != nil { - for _, mux := range muxs { - _ = mux.Close() - } - return nil, err - } - return NewMultiUDPMuxDefault(muxs...), nil } diff --git a/udp_mux_multi_test.go b/udp_mux_multi_test.go index 6644963..82e3e9b 100644 --- a/udp_mux_multi_test.go +++ b/udp_mux_multi_test.go @@ -34,15 +34,12 @@ func TestMultiUDPMux(t *testing.T) { } muxes := []UDPMux{} - muxV41, err := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn1}) - require.NoError(t, err) + muxV41 := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn1}) muxes = append(muxes, muxV41) - muxV42, err := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn2}) - require.NoError(t, err) + muxV42 := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn2}) muxes = append(muxes, muxV42) if conn3 != nil { - muxV6, v6err := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn3}) - require.NoError(t, v6err) + muxV6 := NewUDPMuxDefault(UDPMuxParams{UDPConn: conn3}) muxes = append(muxes, muxV6) } diff --git a/udp_mux_test.go b/udp_mux_test.go index 36192ae..fb6adcf 100644 --- a/udp_mux_test.go +++ b/udp_mux_test.go @@ -62,13 +62,11 @@ func TestUDPMux(t *testing.T) { continue } t.Run(subTest.name, func(t *testing.T) { - udpMux, err := NewUDPMuxDefault(UDPMuxParams{ + udpMux := NewUDPMuxDefault(UDPMuxParams{ Logger: nil, UDPConn: conn, }) - require.NoError(t, err) - defer func() { _ = udpMux.Close() _ = conn.Close() diff --git a/udp_mux_universal.go b/udp_mux_universal.go index 599213e..53e45d9 100644 --- a/udp_mux_universal.go +++ b/udp_mux_universal.go @@ -38,7 +38,7 @@ type UniversalUDPMuxParams struct { } // NewUniversalUDPMuxDefault creates an implementation of UniversalUDPMux embedding UDPMux -func NewUniversalUDPMuxDefault(params UniversalUDPMuxParams) (*UniversalUDPMuxDefault, error) { +func NewUniversalUDPMuxDefault(params UniversalUDPMuxParams) *UniversalUDPMuxDefault { if params.Logger == nil { params.Logger = logging.NewDefaultLoggerFactory().NewLogger("ice") } @@ -64,13 +64,9 @@ func NewUniversalUDPMuxDefault(params UniversalUDPMuxParams) (*UniversalUDPMuxDe Logger: params.Logger, UDPConn: m.params.UDPConn, } - muxDefault, err := NewUDPMuxDefault(udpMuxParams) - if err != nil { - return nil, err - } - m.UDPMuxDefault = muxDefault + m.UDPMuxDefault = NewUDPMuxDefault(udpMuxParams) - return m, nil + return m } // udpConn is a wrapper around UDPMux conn that overrides ReadFrom and handles STUN/TURN packets @@ -107,7 +103,8 @@ func (c *udpConn) ReadFrom(p []byte) (n int, addr net.Addr, err error) { if err = msg.Decode(); err != nil { c.logger.Warnf("Failed to handle decode ICE from %s: %v", addr.String(), err) - return n, addr, nil + err = nil + return } udpAddr, ok := addr.(*net.UDPAddr) @@ -120,12 +117,12 @@ func (c *udpConn) ReadFrom(p []byte) (n int, addr net.Addr, err error) { err = c.mux.handleXORMappedResponse(udpAddr, msg) if err != nil { c.logger.Debugf("%w: %v", errGetXorMappedAddrResponse, err) - return n, addr, nil + err = nil } return } } - return n, addr, err + return } // isXORMappedResponse indicates whether the message is a XORMappedAddress and is coming from the known STUN server. diff --git a/udp_mux_universal_test.go b/udp_mux_universal_test.go index 791e171..2e6d8a8 100644 --- a/udp_mux_universal_test.go +++ b/udp_mux_universal_test.go @@ -17,12 +17,11 @@ func TestUniversalUDPMux(t *testing.T) { conn, err := net.ListenUDP(udp, &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1)}) require.NoError(t, err) - udpMux, err := NewUniversalUDPMuxDefault(UniversalUDPMuxParams{ + udpMux := NewUniversalUDPMuxDefault(UniversalUDPMuxParams{ Logger: nil, UDPConn: conn, }) - require.NoError(t, err) defer func() { _ = udpMux.Close() _ = conn.Close()