From 2250eaa91a43accda44894b3ef96904f7e6df426 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 10 Jun 2021 15:46:33 +0200 Subject: [PATCH] Apply suggestions from code review --- internal/ptx/obfs4.go | 4 ++-- internal/ptx/obfs4_test.go | 2 +- internal/ptx/ptx.go | 16 +++++++++------- internal/ptx/ptx_test.go | 4 +--- internal/ptx/snowflake.go | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/internal/ptx/obfs4.go b/internal/ptx/obfs4.go index 96ba494d5..56ac972d3 100644 --- a/internal/ptx/obfs4.go +++ b/internal/ptx/obfs4.go @@ -84,7 +84,7 @@ func (d *OBFS4Dialer) newCancellableDialer() (*obfs4CancellableDialer, error) { func (d *OBFS4Dialer) newFactory() base.ClientFactory { o4f := &obfs4.Transport{} cf, err := o4f.ClientFactory(filepath.Join(d.DataDir, "obfs4")) - // the source code for this transport always returns nil + // the source code for this transport always returns a nil error runtimex.PanicOnError(err, "unexpected o4f.ClientFactory failure") return cf } @@ -111,7 +111,7 @@ func (d *OBFS4Dialer) underlyingDialer() UnderlyingDialer { type obfs4CancellableDialer struct { // done is a channel that will be closed when done. In normal // usage you don't want to await for this signal. But it's useful - // for testing to know that the background goroutine joned. + // for testing to know that the background goroutine joined. done chan interface{} // factory is the factory for obfs4. diff --git a/internal/ptx/obfs4_test.go b/internal/ptx/obfs4_test.go index 028b39ce7..62df62056 100644 --- a/internal/ptx/obfs4_test.go +++ b/internal/ptx/obfs4_test.go @@ -13,7 +13,7 @@ import ( func TestOBFS4DialerWorks(t *testing.T) { // This test is 0.3 seconds in my machine, so it's ~fine - // to run it when we're in short mode + // to run it even when we're in short mode o4d := DefaultTestingOBFS4Bridge() conn, err := o4d.DialContext(context.Background()) if err != nil { diff --git a/internal/ptx/ptx.go b/internal/ptx/ptx.go index e78873461..760ffd193 100644 --- a/internal/ptx/ptx.go +++ b/internal/ptx/ptx.go @@ -64,7 +64,7 @@ type PTDialer interface { } // Listener is a generic pluggable transports listener. Make sure -// you fill the mandatory fields before using. Do not modify public +// you fill the mandatory fields before using it. Do not modify public // fields after you called Start, since this causes data races. type Listener struct { // PTDialer is the MANDATORY pluggable transports dialer @@ -80,7 +80,7 @@ type Listener struct { // mu provides mutual exclusion for accessing internals. mu sync.Mutex - // cancel allows to stop the forwarders. + // cancel allows stopping the forwarders. cancel context.CancelFunc // laddr is the listen address. @@ -149,7 +149,7 @@ func (lst *Listener) handleSocksConn(ctx context.Context, socksConn ptxSocksConn lst.logger().Warnf("ptx: ContextDialer.DialContext error: %s", err) return err // used for testing } - lst.forwardWithContext(ctx, socksConn, ptConn) + lst.forwardWithContext(ctx, socksConn, ptConn) // transfer ownership return nil // used for testing } @@ -170,7 +170,7 @@ type ptxSocksConn interface { // net.Conn is the embedded interface. net.Conn - // Grants access to a specific IP address. + // Grant grants access to a specific IP address. Grant(addr *net.TCPAddr) error } @@ -184,7 +184,7 @@ func (lst *Listener) acceptLoop(ctx context.Context, ln ptxSocksListener) { continue } lst.logger().Warnf("ptx: socks accept error: %s", err) - break + return } go lst.handleSocksConn(ctx, conn) } @@ -193,7 +193,7 @@ func (lst *Listener) acceptLoop(ctx context.Context, ln ptxSocksListener) { // Addr returns the listening address. This function should not // be called after you have called the Stop method or before the // Start method has successfully returned. When invoked in such -// conditions, this function will return nil. Otherwise, it will +// conditions, this function may return nil. Otherwise, it will // return the valid net.Addr where we are listening. func (lst *Listener) Addr() net.Addr { return lst.laddr @@ -250,7 +250,9 @@ func (la *ptxSocksListenerAdapter) AcceptSocks() (ptxSocksConn, error) { return la.SocksListener.AcceptSocks() } -// torCmdLine prints the command line for testing this listener. +// torCmdLine prints the command line for testing this listener. This method is here to +// facilitate debugging with `ptxclient`, so there is no need to be too precise with arguments +// quoting. Remember to improve upon this aspect if you plan on using it beyond testing. func (lst *Listener) torCmdLine() string { return strings.Join([]string{ "tor", diff --git a/internal/ptx/ptx_test.go b/internal/ptx/ptx_test.go index 8c00b17d8..c7ef744fd 100644 --- a/internal/ptx/ptx_test.go +++ b/internal/ptx/ptx_test.go @@ -103,7 +103,7 @@ type mockableSocksConn struct { MockGrant func(addr *net.TCPAddr) error } -// Grants access to a specific IP address. +// Grant grants access to a specific IP address. func (c *mockableSocksConn) Grant(addr *net.TCPAddr) error { return c.MockGrant(addr) } @@ -180,7 +180,6 @@ func TestListenerForwardWithContextWithContextCancellation(t *testing.T) { lst := &Listener{} left, right := net.Pipe() go lst.forwardWithContext(ctx, left, right) - time.Sleep(100 * time.Millisecond) cancel() } @@ -188,7 +187,6 @@ func TestListenerForwardWithNaturalTermination(t *testing.T) { lst := &Listener{} left, right := net.Pipe() go lst.forwardWithContext(context.Background(), left, right) - time.Sleep(100 * time.Millisecond) right.Close() } diff --git a/internal/ptx/snowflake.go b/internal/ptx/snowflake.go index 9fef5690c..117ef82d0 100644 --- a/internal/ptx/snowflake.go +++ b/internal/ptx/snowflake.go @@ -58,7 +58,7 @@ func (d *SnowflakeDialer) dialContext( } connch, errch := make(chan net.Conn), make(chan error, 1) go func() { - defer close(done) // allow tests to synchronize with our end + defer close(done) // allow tests to synchronize with this goroutine's exit conn, err := txp.Dial() if err != nil { errch <- err // buffered channel @@ -90,8 +90,8 @@ func (d *SnowflakeDialer) newSnowflakeClient(brokerURL string, frontDomain strin keepLocalAddresses, maxSnowflakes) } return sflib.NewSnowflakeClient( - d.brokerURL(), d.frontDomain(), d.iceAddresses(), - false, d.maxSnowflakes()) + brokerURL, frontDomain, iceAddresses, + keepLocalAddresses, maxSnowflakes) } // brokerURL returns a suitable broker URL.