[sys] windows/namedpipe: add simple named pipe library
Jason A. Donenfeld (Gerrit)
Jason A. Donenfeld has uploaded this change for review.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/pipe.go
3 files changed, 819 insertions(+), 0 deletions(-)
diff --git a/windows/namedpipe/file.go b/windows/namedpipe/file.go
new file mode 100644
index 0000000..3dd1263
--- /dev/null
+++ b/windows/namedpipe/file.go
@@ -0,0 +1,297 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build windows
+
+package namedpipe
+
+import (
+ "io"
+ "os"
+ "runtime"
+ "sync"
+ "sync/atomic"
+ "time"
+ "unsafe"
+
+ "golang.org/x/sys/windows"
+)
+
+type atomicBool int32
+
+func (b *atomicBool) isSet() bool { return atomic.LoadInt32((*int32)(b)) != 0 }
+func (b *atomicBool) setFalse() { atomic.StoreInt32((*int32)(b), 0) }
+func (b *atomicBool) setTrue() { atomic.StoreInt32((*int32)(b), 1) }
+func (b *atomicBool) swap(new bool) bool {
+ var newInt int32
+ if new {
+ newInt = 1
+ }
+ return atomic.SwapInt32((*int32)(b), newInt) == 1
+}
+
+type timeoutChan chan struct{}
+
+var ioInitOnce sync.Once
+var ioCompletionPort windows.Handle
+
+// ioResult contains the result of an asynchronous IO operation
+type ioResult struct {
+ bytes uint32
+ err error
+}
+
+// ioOperation represents an outstanding asynchronous Win32 IO
+type ioOperation struct {
+ o windows.Overlapped
+ ch chan ioResult
+}
+
+func initIo() {
+ h, err := windows.CreateIoCompletionPort(windows.InvalidHandle, 0, 0, 0xffffffff)
+ if err != nil {
+ panic(err)
+ }
+ ioCompletionPort = h
+ go ioCompletionProcessor(h)
+}
+
+// win32File implements Reader, Writer, and Closer on a Win32 handle without blocking in a syscall.
+// It takes ownership of this handle and will close it if it is garbage collected.
+type win32File struct {
+ handle windows.Handle
+ wg sync.WaitGroup
+ wgLock sync.RWMutex
+ closing atomicBool
+ socket bool
+ readDeadline deadlineHandler
+ writeDeadline deadlineHandler
+}
+
+type deadlineHandler struct {
+ setLock sync.Mutex
+ channel timeoutChan
+ channelLock sync.RWMutex
+ timer *time.Timer
+ timedout atomicBool
+}
+
+// makeWin32File makes a new win32File from an existing file handle
+func makeWin32File(h windows.Handle) (*win32File, error) {
+ f := &win32File{handle: h}
+ ioInitOnce.Do(initIo)
+ _, err := windows.CreateIoCompletionPort(h, ioCompletionPort, 0, 0xffffffff)
+ if err != nil {
+ return nil, err
+ }
+ err = windows.SetFileCompletionNotificationModes(h, windows.FILE_SKIP_COMPLETION_PORT_ON_SUCCESS|windows.FILE_SKIP_SET_EVENT_ON_HANDLE)
+ if err != nil {
+ return nil, err
+ }
+ f.readDeadline.channel = make(timeoutChan)
+ f.writeDeadline.channel = make(timeoutChan)
+ return f, nil
+}
+
+// closeHandle closes the resources associated with a Win32 handle
+func (f *win32File) closeHandle() {
+ f.wgLock.Lock()
+ // Atomically set that we are closing, releasing the resources only once.
+ if !f.closing.swap(true) {
+ f.wgLock.Unlock()
+ // cancel all IO and wait for it to complete
+ windows.CancelIoEx(f.handle, nil)
+ f.wg.Wait()
+ // at this point, no new IO can start
+ windows.Close(f.handle)
+ f.handle = 0
+ } else {
+ f.wgLock.Unlock()
+ }
+}
+
+// Close closes a win32File.
+func (f *win32File) Close() error {
+ f.closeHandle()
+ return nil
+}
+
+// prepareIo prepares for a new IO operation.
+// The caller must call f.wg.Done() when the IO is finished, prior to Close() returning.
+func (f *win32File) prepareIo() (*ioOperation, error) {
+ f.wgLock.RLock()
+ if f.closing.isSet() {
+ f.wgLock.RUnlock()
+ return nil, os.ErrClosed
+ }
+ f.wg.Add(1)
+ f.wgLock.RUnlock()
+ c := &ioOperation{}
+ c.ch = make(chan ioResult)
+ return c, nil
+}
+
+// ioCompletionProcessor processes completed async IOs forever
+func ioCompletionProcessor(h windows.Handle) {
+ for {
+ var bytes uint32
+ var key uintptr
+ var op *ioOperation
+ err := windows.GetQueuedCompletionStatus(h, &bytes, &key, (**windows.Overlapped)(unsafe.Pointer(&op)), windows.INFINITE)
+ if op == nil {
+ panic(err)
+ }
+ op.ch <- ioResult{bytes, err}
+ }
+}
+
+// asyncIo processes the return value from ReadFile or WriteFile, blocking until
+// the operation has actually completed.
+func (f *win32File) asyncIo(c *ioOperation, d *deadlineHandler, bytes uint32, err error) (int, error) {
+ if err != windows.ERROR_IO_PENDING {
+ return int(bytes), err
+ }
+
+ if f.closing.isSet() {
+ windows.CancelIoEx(f.handle, &c.o)
+ }
+
+ var timeout timeoutChan
+ if d != nil {
+ d.channelLock.Lock()
+ timeout = d.channel
+ d.channelLock.Unlock()
+ }
+
+ var r ioResult
+ select {
+ case r = <-c.ch:
+ err = r.err
+ if err == windows.ERROR_OPERATION_ABORTED {
+ if f.closing.isSet() {
+ err = os.ErrClosed
+ }
+ } else if err != nil && f.socket {
+ // err is from Win32. Query the overlapped structure to get the winsock error.
+ var bytes, flags uint32
+ err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)
+ }
+ case <-timeout:
+ windows.CancelIoEx(f.handle, &c.o)
+ r = <-c.ch
+ err = r.err
+ if err == windows.ERROR_OPERATION_ABORTED {
+ err = os.ErrDeadlineExceeded
+ }
+ }
+
+ // runtime.KeepAlive is needed, as c is passed via native
+ // code to ioCompletionProcessor, c must remain alive
+ // until the channel read is complete.
+ runtime.KeepAlive(c)
+ return int(r.bytes), err
+}
+
+// Read reads from a file handle.
+func (f *win32File) Read(b []byte) (int, error) {
+ c, err := f.prepareIo()
+ if err != nil {
+ return 0, err
+ }
+ defer f.wg.Done()
+
+ if f.readDeadline.timedout.isSet() {
+ return 0, os.ErrDeadlineExceeded
+ }
+
+ var bytes uint32
+ err = windows.ReadFile(f.handle, b, &bytes, &c.o)
+ n, err := f.asyncIo(c, &f.readDeadline, bytes, err)
+ runtime.KeepAlive(b)
+
+ // Handle EOF conditions.
+ if err == nil && n == 0 && len(b) != 0 {
+ return 0, io.EOF
+ } else if err == windows.ERROR_BROKEN_PIPE {
+ return 0, io.EOF
+ } else {
+ return n, err
+ }
+}
+
+// Write writes to a file handle.
+func (f *win32File) Write(b []byte) (int, error) {
+ c, err := f.prepareIo()
+ if err != nil {
+ return 0, err
+ }
+ defer f.wg.Done()
+
+ if f.writeDeadline.timedout.isSet() {
+ return 0, os.ErrDeadlineExceeded
+ }
+
+ var bytes uint32
+ err = windows.WriteFile(f.handle, b, &bytes, &c.o)
+ n, err := f.asyncIo(c, &f.writeDeadline, bytes, err)
+ runtime.KeepAlive(b)
+ return n, err
+}
+
+func (f *win32File) SetReadDeadline(deadline time.Time) error {
+ return f.readDeadline.set(deadline)
+}
+
+func (f *win32File) SetWriteDeadline(deadline time.Time) error {
+ return f.writeDeadline.set(deadline)
+}
+
+func (f *win32File) Flush() error {
+ return windows.FlushFileBuffers(f.handle)
+}
+
+func (f *win32File) Fd() uintptr {
+ return uintptr(f.handle)
+}
+
+func (d *deadlineHandler) set(deadline time.Time) error {
+ d.setLock.Lock()
+ defer d.setLock.Unlock()
+
+ if d.timer != nil {
+ if !d.timer.Stop() {
+ <-d.channel
+ }
+ d.timer = nil
+ }
+ d.timedout.setFalse()
+
+ select {
+ case <-d.channel:
+ d.channelLock.Lock()
+ d.channel = make(chan struct{})
+ d.channelLock.Unlock()
+ default:
+ }
+
+ if deadline.IsZero() {
+ return nil
+ }
+
+ timeoutIO := func() {
+ d.timedout.setTrue()
+ close(d.channel)
+ }
+
+ now := time.Now()
+ duration := deadline.Sub(now)
+ if deadline.After(now) {
+ // Deadline is in the future, set a timer to wait
+ d.timer = time.AfterFunc(duration, timeoutIO)
+ } else {
+ // Deadline is in the past. Cancel all pending IO now.
+ timeoutIO()
+ }
+ return nil
+}
diff --git a/windows/namedpipe/namedpipe_test.go b/windows/namedpipe/namedpipe_test.go
new file mode 100644
index 0000000..82a8e54
--- /dev/null
+++ b/windows/namedpipe/namedpipe_test.go
@@ -0,0 +1,66 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build windows
+
+package namedpipe_test
+
+import (
+ "fmt"
+ "math/rand"
+ "testing"
+
+ "golang.org/x/sys/windows/namedpipe"
+)
+
+func TestDialAndListen(t *testing.T) {
+ const (
+ ping = 42
+ pong = 24
+ )
+ pipePath := fmt.Sprintf(`\\.\pipe\go-test-%d-%d`, rand.Uint64(), rand.Uint64())
+ listener, err := namedpipe.Listen(pipePath, nil)
+ if err != nil {
+ t.Errorf("unable to listen on pipe: %v", err)
+ }
+ defer listener.Close()
+ go func() {
+ incoming, err := listener.Accept()
+ if err != nil {
+ t.Errorf("unable to accept pipe connection: %v", err)
+ }
+ defer incoming.Close()
+ var data [1]byte
+ _, err = incoming.Read(data[:])
+ if err != nil {
+ t.Errorf("unable to read ping from pipe: %v", err)
+ }
+ if data[0] != ping {
+ t.Errorf("expected ping, got %d", data[0])
+ }
+ data[0] = pong
+ _, err = incoming.Write(data[:])
+ if err != nil {
+ t.Errorf("unable to write pong to pipe: %v", err)
+ }
+ }()
+ client, err := namedpipe.Dial(pipePath, nil, nil)
+ if err != nil {
+ t.Errorf("unable to dial pipe: %v", err)
+ }
+ defer client.Close()
+ var data [1]byte
+ data[0] = ping
+ _, err = client.Write(data[:])
+ if err != nil {
+ t.Errorf("unable to write ping to pipe: %v", err)
+ }
+ _, err = client.Read(data[:])
+ if err != nil {
+ t.Errorf("unable to read pong from pipe: %v", err)
+ }
+ if data[0] != pong {
+ t.Errorf("expected pong, got %d", data[0])
+ }
+}
diff --git a/windows/namedpipe/pipe.go b/windows/namedpipe/pipe.go
new file mode 100644
index 0000000..5335ee9
--- /dev/null
+++ b/windows/namedpipe/pipe.go
@@ -0,0 +1,456 @@
+// Copyright 2021 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build windows
+
+package namedpipe
+
+import (
+ "context"
+ "fmt"
+ "io"
+ "net"
+ "os"
+ "runtime"
+ "time"
+ "unsafe"
+
+ "golang.org/x/sys/windows"
+)
+
+type win32Pipe struct {
+ *win32File
+ path string
+}
+
+type win32MessageBytePipe struct {
+ win32Pipe
+ writeClosed bool
+ readEOF bool
+}
+
+type pipeAddress string
+
+func (f *win32Pipe) LocalAddr() net.Addr {
+ return pipeAddress(f.path)
+}
+
+func (f *win32Pipe) RemoteAddr() net.Addr {
+ return pipeAddress(f.path)
+}
+
+func (f *win32Pipe) SetDeadline(t time.Time) error {
+ f.SetReadDeadline(t)
+ f.SetWriteDeadline(t)
+ return nil
+}
+
+// CloseWrite closes the write side of a message pipe in byte mode.
+func (f *win32MessageBytePipe) CloseWrite() error {
+ if f.writeClosed {
+ return io.ErrClosedPipe
+ }
+ err := f.win32File.Flush()
+ if err != nil {
+ return err
+ }
+ _, err = f.win32File.Write(nil)
+ if err != nil {
+ return err
+ }
+ f.writeClosed = true
+ return nil
+}
+
+// Write writes bytes to a message pipe in byte mode. Zero-byte writes are ignored, since
+// they are used to implement CloseWrite().
+func (f *win32MessageBytePipe) Write(b []byte) (int, error) {
+ if f.writeClosed {
+ return 0, io.ErrClosedPipe
+ }
+ if len(b) == 0 {
+ return 0, nil
+ }
+ return f.win32File.Write(b)
+}
+
+// Read reads bytes from a message pipe in byte mode. A read of a zero-byte message on a message
+// mode pipe will return io.EOF, as will all subsequent reads.
+func (f *win32MessageBytePipe) Read(b []byte) (int, error) {
+ if f.readEOF {
+ return 0, io.EOF
+ }
+ n, err := f.win32File.Read(b)
+ if err == io.EOF {
+ // If this was the result of a zero-byte read, then
+ // it is possible that the read was due to a zero-size
+ // message. Since we are simulating CloseWrite with a
+ // zero-byte message, ensure that all future Read() calls
+ // also return EOF.
+ f.readEOF = true
+ } else if err == windows.ERROR_MORE_DATA {
+ // ERROR_MORE_DATA indicates that the pipe's read mode is message mode
+ // and the message still has more bytes. Treat this as a success, since
+ // this package presents all named pipes as byte streams.
+ err = nil
+ }
+ return n, err
+}
+
+func (s pipeAddress) Network() string {
+ return "pipe"
+}
+
+func (s pipeAddress) String() string {
+ return string(s)
+}
+
+// tryDialPipe attempts to dial the pipe at `path` until `ctx` cancellation or timeout.
+func tryDialPipe(ctx context.Context, path *string) (windows.Handle, error) {
+ for {
+ select {
+ case <-ctx.Done():
+ return 0, ctx.Err()
+ default:
+ path16, err := windows.UTF16PtrFromString(*path)
+ if err != nil {
+ return 0, err
+ }
+ h, err := windows.CreateFile(path16, windows.GENERIC_READ|windows.GENERIC_WRITE, 0, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_OVERLAPPED|windows.SECURITY_SQOS_PRESENT|windows.SECURITY_ANONYMOUS, 0)
+ if err == nil {
+ return h, nil
+ }
+ if err != windows.ERROR_PIPE_BUSY {
+ return h, &os.PathError{Err: err, Op: "open", Path: *path}
+ }
+ // Wait 10 msec and try again. This is a rather simplistic
+ // view, as we always try each 10 milliseconds.
+ time.Sleep(time.Millisecond * 10)
+ }
+ }
+}
+
+// Dial connects to a named pipe by path, timing out if the connection
+// takes longer than the specified duration. If timeout is nil, then we use
+// a default timeout of 2 seconds. If an expected owner SID is not nil, then
+// the pipe is verified to be owned by that SID.
+func Dial(path string, timeout *time.Duration, expectedOwner *windows.SID) (net.Conn, error) {
+ var absTimeout time.Time
+ if timeout != nil {
+ absTimeout = time.Now().Add(*timeout)
+ } else {
+ absTimeout = time.Now().Add(time.Second * 2)
+ }
+ ctx, _ := context.WithDeadline(context.Background(), absTimeout)
+ conn, err := DialContext(ctx, path, expectedOwner)
+ if err == context.DeadlineExceeded {
+ return nil, os.ErrDeadlineExceeded
+ }
+ return conn, err
+}
+
+// DialContext attempts to connect to a named pipe by `path` until `ctx`
+// cancellation or timeout. If an expected owner SID is not nil, then
+// // the pipe is verified to be owned by that SID.
+func DialContext(ctx context.Context, path string, expectedOwner *windows.SID) (net.Conn, error) {
+ var err error
+ var h windows.Handle
+ h, err = tryDialPipe(ctx, &path)
+ if err != nil {
+ return nil, err
+ }
+
+ if expectedOwner != nil {
+ sd, err := windows.GetSecurityInfo(h, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION)
+ if err != nil {
+ windows.Close(h)
+ return nil, err
+ }
+ realOwner, _, err := sd.Owner()
+ if err != nil {
+ windows.Close(h)
+ return nil, err
+ }
+ if !realOwner.Equals(expectedOwner) {
+ windows.Close(h)
+ return nil, windows.ERROR_ACCESS_DENIED
+ }
+ }
+
+ var flags uint32
+ err = windows.GetNamedPipeInfo(h, &flags, nil, nil, nil)
+ if err != nil {
+ windows.Close(h)
+ return nil, err
+ }
+
+ f, err := makeWin32File(h)
+ if err != nil {
+ windows.Close(h)
+ return nil, err
+ }
+
+ // If the pipe is in message mode, return a message byte pipe, which
+ // supports CloseWrite().
+ if flags&windows.PIPE_TYPE_MESSAGE != 0 {
+ return &win32MessageBytePipe{
+ win32Pipe: win32Pipe{win32File: f, path: path},
+ }, nil
+ }
+ return &win32Pipe{win32File: f, path: path}, nil
+}
+
+type acceptResponse struct {
+ f *win32File
+ err error
+}
+
+type win32PipeListener struct {
+ firstHandle windows.Handle
+ path string
+ config PipeConfig
+ acceptCh chan (chan acceptResponse)
+ closeCh chan int
+ doneCh chan int
+}
+
+func makeServerPipeHandle(path string, sd *windows.SECURITY_DESCRIPTOR, c *PipeConfig, first bool) (windows.Handle, error) {
+ path16, err := windows.UTF16FromString(path)
+ if err != nil {
+ return 0, &os.PathError{Op: "open", Path: path, Err: err}
+ }
+
+ var oa windows.OBJECT_ATTRIBUTES
+ oa.Length = uint32(unsafe.Sizeof(oa))
+
+ var ntPath windows.UNICODE_STRING
+ if err := windows.RtlDosPathNameToNtPathName(&path16[0], &ntPath, nil, nil); err != nil {
+ if ntstatus, ok := err.(windows.NTStatus); ok {
+ err = ntstatus.Errno()
+ }
+ return 0, &os.PathError{Op: "open", Path: path, Err: err}
+ }
+ defer windows.LocalFree(windows.Handle(unsafe.Pointer(ntPath.Buffer)))
+ oa.ObjectName = &ntPath
+
+ // The security descriptor is only needed for the first pipe.
+ if first {
+ if sd != nil {
+ oa.SecurityDescriptor = sd
+ } else {
+ // Construct the default named pipe security descriptor.
+ var dacl *windows.ACL
+ if err := windows.RtlDefaultNpAcl(&dacl); err != nil {
+ return 0, fmt.Errorf("failed to get default named pipe ACL: %v", err)
+ }
+ defer windows.LocalFree(windows.Handle(unsafe.Pointer(dacl)))
+ sd, err := windows.NewSecurityDescriptor()
+ if err != nil {
+ return 0, fmt.Errorf("creating new security descriptor: %s", err)
+ }
+ if err = sd.SetDACL((*windows.ACL)(unsafe.Pointer(dacl)), true, false); err != nil {
+ return 0, fmt.Errorf("assigning dacl: %s", err)
+ }
+ sd, err = sd.ToSelfRelative()
+ if err != nil {
+ return 0, fmt.Errorf("converting to self-relative: %s", err)
+ }
+ oa.SecurityDescriptor = sd
+ }
+ }
+
+ typ := uint32(windows.FILE_PIPE_REJECT_REMOTE_CLIENTS)
+ if c.MessageMode {
+ typ |= windows.FILE_PIPE_MESSAGE_TYPE
+ }
+
+ disposition := uint32(windows.FILE_OPEN)
+ access := uint32(windows.GENERIC_READ | windows.GENERIC_WRITE | windows.SYNCHRONIZE)
+ if first {
+ disposition = windows.FILE_CREATE
+ // By not asking for read or write access, the named pipe file system
+ // will put this pipe into an initially disconnected state, blocking
+ // client connections until the next call with first == false.
+ access = windows.SYNCHRONIZE
+ }
+
+ timeout := int64(-50 * 10000) // 50ms
+
+ var (
+ h windows.Handle
+ iosb windows.IO_STATUS_BLOCK
+ )
+ err = windows.NtCreateNamedPipeFile(&h, access, &oa, &iosb, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, disposition, 0, typ, 0, 0, 0xffffffff, uint32(c.InputBufferSize), uint32(c.OutputBufferSize), &timeout)
+ if err != nil {
+ if ntstatus, ok := err.(windows.NTStatus); ok {
+ err = ntstatus.Errno()
+ }
+ return 0, &os.PathError{Op: "open", Path: path, Err: err}
+ }
+
+ runtime.KeepAlive(ntPath)
+ return h, nil
+}
+
+func (l *win32PipeListener) makeServerPipe() (*win32File, error) {
+ h, err := makeServerPipeHandle(l.path, nil, &l.config, false)
+ if err != nil {
+ return nil, err
+ }
+ f, err := makeWin32File(h)
+ if err != nil {
+ windows.Close(h)
+ return nil, err
+ }
+ return f, nil
+}
+
+func (l *win32PipeListener) makeConnectedServerPipe() (*win32File, error) {
+ p, err := l.makeServerPipe()
+ if err != nil {
+ return nil, err
+ }
+
+ // Wait for the client to connect.
+ ch := make(chan error)
+ go func(p *win32File) {
+ ch <- connectPipe(p)
+ }(p)
+
+ select {
+ case err = <-ch:
+ if err != nil {
+ p.Close()
+ p = nil
+ }
+ case <-l.closeCh:
+ // Abort the connect request by closing the handle.
+ p.Close()
+ p = nil
+ err = <-ch
+ if err == nil || err == os.ErrClosed {
+ err = net.ErrClosed
+ }
+ }
+ return p, err
+}
+
+func (l *win32PipeListener) listenerRoutine() {
+ closed := false
+ for !closed {
+ select {
+ case <-l.closeCh:
+ closed = true
+ case responseCh := <-l.acceptCh:
+ var (
+ p *win32File
+ err error
+ )
+ for {
+ p, err = l.makeConnectedServerPipe()
+ // If the connection was immediately closed by the client, try
+ // again.
+ if err != windows.ERROR_NO_DATA {
+ break
+ }
+ }
+ responseCh <- acceptResponse{p, err}
+ closed = err == net.ErrClosed
+ }
+ }
+ windows.Close(l.firstHandle)
+ l.firstHandle = 0
+ // Notify Close() and Accept() callers that the handle has been closed.
+ close(l.doneCh)
+}
+
+// PipeConfig contain configuration for the pipe listener.
+type PipeConfig struct {
+ // SecurityDescriptor contains a Windows security descriptor.
+ SecurityDescriptor *windows.SECURITY_DESCRIPTOR
+
+ // MessageMode determines whether the pipe is in byte or message mode. In either
+ // case the pipe is read in byte mode by default. The only practical difference in
+ // this implementation is that CloseWrite() is only supported for message mode pipes;
+ // CloseWrite() is implemented as a zero-byte write, but zero-byte writes are only
+ // transferred to the reader (and returned as io.EOF in this implementation)
+ // when the pipe is in message mode.
+ MessageMode bool
+
+ // InputBufferSize specifies the size the input buffer, in bytes.
+ InputBufferSize int32
+
+ // OutputBufferSize specifies the size the input buffer, in bytes.
+ OutputBufferSize int32
+}
+
+// Listen creates a listener on a Windows named pipe path, e.g. \\.\pipe\mypipe.
+// The pipe must not already exist.
+func Listen(path string, c *PipeConfig) (net.Listener, error) {
+ if c == nil {
+ c = &PipeConfig{}
+ }
+ h, err := makeServerPipeHandle(path, c.SecurityDescriptor, c, true)
+ if err != nil {
+ return nil, err
+ }
+ l := &win32PipeListener{
+ firstHandle: h,
+ path: path,
+ config: *c,
+ acceptCh: make(chan (chan acceptResponse)),
+ closeCh: make(chan int),
+ doneCh: make(chan int),
+ }
+ go l.listenerRoutine()
+ return l, nil
+}
+
+func connectPipe(p *win32File) error {
+ c, err := p.prepareIo()
+ if err != nil {
+ return err
+ }
+ defer p.wg.Done()
+
+ err = windows.ConnectNamedPipe(p.handle, &c.o)
+ _, err = p.asyncIo(c, nil, 0, err)
+ if err != nil && err != windows.ERROR_PIPE_CONNECTED {
+ return err
+ }
+ return nil
+}
+
+func (l *win32PipeListener) Accept() (net.Conn, error) {
+ ch := make(chan acceptResponse)
+ select {
+ case l.acceptCh <- ch:
+ response := <-ch
+ err := response.err
+ if err != nil {
+ return nil, err
+ }
+ if l.config.MessageMode {
+ return &win32MessageBytePipe{
+ win32Pipe: win32Pipe{win32File: response.f, path: l.path},
+ }, nil
+ }
+ return &win32Pipe{win32File: response.f, path: l.path}, nil
+ case <-l.doneCh:
+ return nil, net.ErrClosed
+ }
+}
+
+func (l *win32PipeListener) Close() error {
+ select {
+ case l.closeCh <- 1:
+ <-l.doneCh
+ case <-l.doneCh:
+ }
+ return nil
+}
+
+func (l *win32PipeListener) Addr() net.Addr {
+ return pipeAddress(l.path)
+}
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 1:Run-TryBot +1Trust +1
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Jason A. Donenfeld removed a vote from this change.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Jason A. Donenfeld removed a vote from this change.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 1:Run-TryBot +1
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Set Ready For Review
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 5:Run-TryBot +1Trust +1
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Ian Lance Taylor.
6 comments:
Patchset:
(superficial quick review only)
File windows/namedpipe/pipe.go:
Patch Set #5, Line 7:
package namedpipethe main file should have a package doc.
and I'd probably call it namedpipe.go to match the package name, which is somewhat conventional.
Patch Set #5, Line 152:
// DialContext attempts to connect to a named pipe by `path` until `ctx`the backtick notation isn't done anywhere else in Go's trees
Patch Set #5, Line 359:
// Notify Close() and Accept() callers that the handle has been closed.drop the parens unless you're talking about the result of calling them, which you're not here.
Patch Set #5, Line 364:
type PipeConfig struct {specify the meaning of zero values where appropriate.
Patch Set #5, Line 383:
// Listen creates a listener on a Windows named pipe path, e.g. \\.\pipe\mypipe.Go avoids Latin abbreviations in docs. Spell it out in English. For example. Such as.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
6 comments:
Patchset:
(superficial quick review only)
Thanks. There are likely a lot of superficial things to repair in this, because the base code originally back many years.
File windows/namedpipe/pipe.go:
Patch Set #5, Line 7:
package namedpipe
the main file should have a package doc. […]
Done
Patch Set #5, Line 152:
// DialContext attempts to connect to a named pipe by `path` until `ctx`the backtick notation isn't done anywhere else in Go's trees
Done
Patch Set #5, Line 359:
// Notify Close() and Accept() callers that the handle has been closed.drop the parens unless you're talking about the result of calling them, which you're not here.
Done
Patch Set #5, Line 364:
type PipeConfig struct {specify the meaning of zero values where appropriate.
Done
Patch Set #5, Line 383:
// Listen creates a listener on a Windows named pipe path, e.g. \\.\pipe\mypipe.Go avoids Latin abbreviations in docs. Spell it out in English. For example. Such as.
Done
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Jason A. Donenfeld uploaded patch set #6 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
A windows/namedpipe/namedpipe_test.go
M windows/syscall_windows.go
M windows/zsyscall_windows.go
5 files changed, 1,420 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 6:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor.
Patch set 7:Run-TryBot +1Trust +1
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
1 comment:
Patchset:
Adding John, as a lot of this is based on his code.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
Jason A. Donenfeld uploaded patch set #8 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
A windows/namedpipe/namedpipe_test.go
3 files changed, 1,410 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
Patch set 8:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
Jason A. Donenfeld uploaded patch set #9 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
A windows/namedpipe/namedpipe_test.go
3 files changed, 1,414 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
Patch set 9:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
1 comment:
Patchset:
Hoping this can be reviewed. It could be useful. Alternatively, I can continue to maintain this in my own tree.
Happy to hear opinions either way on the matter.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Brad Fitzpatrick, Ian Lance Taylor, John Starks.
Patch set 9:Trust +1
20 comments:
Patchset:
Hoping this can be reviewed. It could be useful. […]
It is fine to add this code here, if there are other users (other than you, Jason).
Nice package.
Thank you.
Alex
File windows/namedpipe/file.go:
Patch Set #9, Line 163:
// err is from Win32. Query the overlapped structure to get the winsock error.This code is never executed, because f.socket is always false.
File windows/namedpipe/namedpipe.go:
Patch Set #9, Line 62:
f.writeClosed = truef.writeClosed is read in f.Write. Do we need to synchronise access to f.writeClosed? Even f.CloseWrite read f.writeClosed on line 51. So there is a race here. Do we care?
Patch Set #9, Line 162:
timeoutI don't see any DialContext parameters refer to "timeout".
path
// cancellationWhat is "path cancellation"?
s/:=/=/
If somehow code changes in a way that err is defined on line 258, your code will start creating local sd variable instead of updating sd parameter.
File windows/namedpipe/namedpipe_test.go:
Patch Set #9, Line 36:
ping = 42You don't need to change the code. But I am just curious why 42 and 24?
Patch Set #9, Line 48:
t.Fatalf("unable to accept pipe connection: %v", err)Do you know that calling t.Fatalf in goroutine here does not exit the test?
For example, after this change
```
diff --git a/windows/namedpipe/namedpipe_test.go b/windows/namedpipe/namedpipe_test.go
index d1807ae5..ef527d3c 100644
--- a/windows/namedpipe/namedpipe_test.go
+++ b/windows/namedpipe/namedpipe_test.go
@@ -44,6 +44,7 @@ func TestPingPong(t *testing.T) {
defer listener.Close()
go func() {
incoming, err := listener.Accept()
+ t.Fatalf("unable to accept pipe connection: %v", err)
if err != nil {
t.Fatalf("unable to accept pipe connection: %v", err)
}
```This command just hangs
```
test -test.v -test.run=Ping
1 file(s) copied.
=== RUN TestPingPong
namedpipe_test.go:47: unable to accept pipe connection: <nil>
^C
C:\Users\Alex\AppData\Local\Temp>
```Same for other t.Fatal.
Patch Set #9, Line 100:
namedpipe.DialWhat happens if namedpipe.Dial succeeds? Shouldn't you try and close returned pipe?
Patch Set #9, Line 115:
namedpipe.DialContextWhat happens if namedpipe.Dial succeeds? Shouldn't you try and close returned pipe?
Patch Set #9, Line 128:
ch := make(chan error)Please swap lines 128 and 129. I would expect to see
defer l.Close()
immediately after namedpipe.Listen returns.
It is confusing to see
ch := make(chan error)
between these lines.
Patch Set #9, Line 156:
namedpipe.DialWhat happens if namedpipe.Dial succeeds? Shouldn't you try and close returned pipe?
Patch Set #9, Line 216:
panicWhy should we accept code that calls panic here and everywhere else during the test?
Patch Set #9, Line 362:
l.AcceptWhat happens if l.Accept succeeds? Shouldn't you try and close returned pipe?
Patch Set #9, Line 375:
namedpipe.DialWhat happens if namedpipe.Dial succeeds? Shouldn't you try and close returned pipe?
Patch Set #9, Line 376:
if err != os.ErrDeadlineExceeded {How long do we have to wait for naedpipe.Dial to timeout? Perhaps add a comment that says
// This should timeout after 1 second.
or something similar.
Patch Set #9, Line 504:
io.CopyDo we need to check for io.Copy errors?
Patch Set #9, Line 525:
close(clientDone)Should you check that bytes contains payload?
Patch Set #9, Line 641:
var wg sync.WaitGroupWhy using sync.WaitGroup here? Simple channel should do instead. No?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Ian Lance Taylor, John Starks.
8 comments:
File windows/namedpipe/namedpipe.go:
Patch Set #9, Line 139:
type DialConfig struct {In net, we have https://golang.org/pkg/net/#ListenConfig with methods on the config.
Likewise https://golang.org/pkg/net/#Dialer with methods on the Dialer config.
Patch Set #9, Line 146:
func Dial(path string, timeout *time.Duration, config *DialConfig) (net.Conn, error) {This is a new package so we don't really need to support context-less Dial.
We could just drop DialContext and make this one take a context. And drop the timeout. The pointer-to-Duration type is a bit weird. I've never seen that before; we usually just define the meaning of 0 instead, but two optional parameters on here is a little ugly.
I'd move Timeout to the config (like https://golang.org/pkg/net/http/#Client.Timeout) and document the 0 value as meaning something like 10 seconds (if you want some default timeout) and saying negative means no timeout, which we do elsewhere IIRC.
Patch Set #9, Line 165:
config = &DialConfig{}you could have:
var defaultDialer Dialer
func (d *defaultDialer) Dial(ctx context.Context, path string) (...)
func Dial(ctx context.Context, path string) (...) {
return defaultDialer.Dial(ctx, path)
}Patch Set #9, Line 223:
acceptCh chan (chan acceptResponse)drop parens
Patch Set #9, Line 228:
func makeServerPipeHandle(path string, sd *windows.SECURITY_DESCRIPTOR, c *ListenConfig, first bool) (windows.Handle, error) {what is "first"? little docs on this?
Patch Set #9, Line 408:
acceptCh: make(chan (chan acceptResponse)),drop parens
Patch Set #9, Line 412:
// The first connection is swallowed on Windows 7 & 8, so synthesize it.hah, wut? how?
Patch Set #9, Line 445:
response := <-chso the contract here is that if you sent this channel, you absolutely are guaranteed to receive a response on it, right? Looks to be the case.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
27 comments:
Patchset:
Sorry this took so long. Changes coming up right now.
File windows/namedpipe/file.go:
Patch Set #9, Line 163:
// err is from Win32. Query the overlapped structure to get the winsock error.This code is never executed, because f.socket is always false.
Indeed, but I'd like to keep it anyway in case this grows.
File windows/namedpipe/namedpipe.go:
Patch Set #9, Line 62:
f.writeClosed = truef.writeClosed is read in f.Write. Do we need to synchronise access to f.writeClosed? Even f. […]
Nice catch. Fixed.
Patch Set #9, Line 139:
type DialConfig struct {
In net, we have https://golang.org/pkg/net/#ListenConfig with methods on the config. […]
I like that. Adopted.
Patch Set #9, Line 146:
func Dial(path string, timeout *time.Duration, config *DialConfig) (net.Conn, error) {
This is a new package so we don't really need to support context-less Dial. […]
I like having the "simpler" optional function that doesn't use context; I'll make it called DialTimeout. I'll make the zero value default to the default as you suggested. That's indeed nicer than what's here now. That combined with making DialConfig the receiver makes this much cleaner than it is right now.
Patch Set #9, Line 162:
timeoutI don't see any DialContext parameters refer to "timeout".
Done
path
// cancellationWhat is "path cancellation"?
Second line was a copy and paste error from elsewhere. Fixed.
Patch Set #9, Line 165:
config = &DialConfig{}
you could have: […]
Done
Patch Set #9, Line 223:
acceptCh chan (chan acceptResponse)drop parens
Done
Patch Set #9, Line 228:
func makeServerPipeHandle(path string, sd *windows.SECURITY_DESCRIPTOR, c *ListenConfig, first bool) (windows.Handle, error) {what is "first"? little docs on this?
It's mentioned a little lower when the variable is used: "// The security descriptor is only needed for the first pipe.". I'll choose a better variable name.
s/:=/=/ […]
Ooof, subtle one. Fixed.
Patch Set #9, Line 408:
acceptCh: make(chan (chan acceptResponse)),drop parens
Done
Patch Set #9, Line 412:
// The first connection is swallowed on Windows 7 & 8, so synthesize it.hah, wut? how?
Kernel swallows it.
Patch Set #9, Line 445:
response := <-ch
so the contract here is that if you sent this channel, you absolutely are guaranteed to receive a re […]
Ack
File windows/namedpipe/namedpipe_test.go:
Patch Set #9, Line 36:
ping = 42
You don't need to change the code. […]
Douglas Adams I guess.
Patch Set #9, Line 48:
t.Fatalf("unable to accept pipe connection: %v", err)
Do you know that calling t.Fatalf in goroutine here does not exit the test? […]
Thanks, nice catch. I'll make those reads timeout for this situation.
Patch Set #9, Line 100:
namedpipe.Dial
What happens if namedpipe. […]
Nice catch. Fixing this and the other ones like it.
Patch Set #9, Line 115:
namedpipe.DialContext
What happens if namedpipe. […]
Ack
Patch Set #9, Line 128:
ch := make(chan error)
Please swap lines 128 and 129. I would expect to see […]
I agree. Fixed.
Patch Set #9, Line 156:
namedpipe.Dial
What happens if namedpipe. […]
Ack
Patch Set #9, Line 216:
panicWhy should we accept code that calls panic here and everywhere else during the test?
In the helper functions it seems fine. It'll be caught by the test harness properly.
Patch Set #9, Line 362:
l.Accept
What happens if l. […]
Ack
Patch Set #9, Line 375:
namedpipe.Dial
What happens if namedpipe. […]
Ack
Patch Set #9, Line 376:
if err != os.ErrDeadlineExceeded {
How long do we have to wait for naedpipe.Dial to timeout? Perhaps add a comment that says […]
Done
Patch Set #9, Line 504:
io.Copy
Do we need to check for io. […]
Done
Patch Set #9, Line 525:
close(clientDone)Should you check that bytes contains payload?
Done
Patch Set #9, Line 641:
var wg sync.WaitGroup
Why using sync.WaitGroup here? Simple channel should do instead. […]
Generally waitgroup is preferred for synchronizing go routine returns like this.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Jason A. Donenfeld uploaded patch set #10 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,459 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Patch set 10:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
2 comments:
File windows/namedpipe/file.go:
Patch Set #10, Line 2:
// Copyright 2005 MicrosoftIs there anything in this file that is copyright 2005 by Microsoft? Not sure how that could be. Similarly elsewhere.
File windows/namedpipe/namedpipe.go:
Patch Set #10, Line 152:
if timeout != time.Duration(0) {You don't need the explicit conversion to time.Duration here, and Go style is to omit it.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
3 comments:
Patchset:
v+1 coming up.
File windows/namedpipe/file.go:
Patch Set #10, Line 2:
// Copyright 2005 Microsoft
Is there anything in this file that is copyright 2005 by Microsoft? Not sure how that could be. […]
The first revisions of this started out based on some code John wrote that had that at the top. It's since morphed a bunch, but I figured better safe than sorry so I left it in there.
File windows/namedpipe/namedpipe.go:
Patch Set #10, Line 152:
if timeout != time.Duration(0) {You don't need the explicit conversion to time.Duration here, and Go style is to omit it.
Ack
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Jason A. Donenfeld uploaded patch set #11 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,457 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Patch set 11:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Jason A. Donenfeld removed a vote from this change.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Patch set 11:Run-TryBot +1
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #10, Line 2:
// Copyright 2005 Microsoft
The first revisions of this started out based on some code John wrote that had that at the top. […]
My sense here is different: I would rather avoid confusion on the copyright notice. I would prefer to remove this copyright notice unless we clearly need it, and since this must be a different language I don't see how we could need it.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #10, Line 2:
// Copyright 2005 Microsoft
My sense here is different: I would rather avoid confusion on the copyright notice. […]
Oh, boy, definitely a typo on my part, or something that's been retroactively corrected elsewhere, but that should be 2015, not 2005. Fixing. Now I see why you found that implausible. :)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Jason A. Donenfeld uploaded patch set #12 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,457 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Simon Rozman.
Patch set 12:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #10, Line 2:
// Copyright 2005 Microsoft
Oh, boy, definitely a typo on my part, or something that's been retroactively corrected elsewhere, b […]
OK, that makes more sense. Thanks.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
Patchset:
Hoping this can be +2'd and submitted soon?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
Jason A. Donenfeld uploaded patch set #13 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,462 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
Patch set 13:Run-TryBot +1Trust +1
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
8 comments:
Patchset:
This is pretty reasonable on the Win32 side, however I do have a few comments.
File windows/namedpipe/file.go:
Let's add:
var ioCompletionKey uintptrand set it to a random number in initIo().
Patch Set #13, Line 41:
h, err := windows.CreateIoCompletionPort(windows.InvalidHandle, 0, 0, 0)If we're only going to start one ioCompletionProcessor goroutine, then we should set 1 as the final argument to CreateIoCompletionPort.
Patch Set #13, Line 45:
ioCompletionPort = hSet ioCompletionKey to a (preferably cryptographically safe) random number.
Patch Set #13, Line 73:
_, err := windows.CreateIoCompletionPort(h, ioCompletionPort, 0, 0)I think it would be worthwhile to specify a non-zero completion key. Referencing my above comments, let's set the completion key to the random number that we saved to ioCompletionKey above. This will help to distinguish legit I/O completions from other garbage that could theoretically be posted to the IOCP by an injected DLL or some other bad actor.
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {GetQueuedCompletionStatus associates the current OS thread with the IOCP. We should LockOSThread this entire function.
It's not fatal to omit it, but if the goroutine ends up being bounced around across multiple OS threads, each GetQueuedCompletionStatus call will re-associate the port with a different OS thread and will probably confuse the hell out of the kernel and hurt performance.
Patch Set #13, Line 132:
panic(err)I am not sure we should unconditionally panic in this case.
When err != nil && op == nil, then err is the code for a failure that occurred inside GetQueuedCompletionStatus itself. Maybe that is panic-worthy.
err == nil && op == nil is possible if something called PostQueuedCompletionStatus on our port with a nil OVERLAPPED. If something else in our process (say an injected DLL) decided to post a nil OVERLAPPED, then they could trigger a panic. By first checking that key == ioCompletionKey, we are adding a bit of entropy to provide a rudimentary defense against this. Only if the key matches should we then check op. Anything that doesn't match our key should just be ignored and we should continue looping in that case.
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)I don't think that this code is necessary. When GetQueuedCompletionStatus returns err != nil and OVERLAPPED != nil, then it already produced the same results as if we had called GetOverlappedResult: err is already the code for the failure in the I/O operation.
Furthermore, sockets are just regular kernel handles and WSA* errors live in the same number space as regular Win32 errors anyway, so I don't see any advantage to having this special code path for sockets.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
4 comments:
Patchset:
A few more remarks here.
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 254:
defer windows.LocalFree(windows.Handle(unsafe.Pointer(ntPath.Buffer)))HeapFree(GetProcessHeap(), 0, unsafe.Pointer(ntPath.Buffer)) is the more appropriate API to use here IMHO. It looks like neither of those APIs are currently in sys/windows though :-(
Patch Set #13, Line 267:
defer windows.LocalFree(windows.Handle(unsafe.Pointer(acl)))HeapFree(GetProcessHeap(), 0, unsafe.Pointer(acl)) is the more appropriate API to use here IMHO. It looks like neither of those APIs are currently in sys/windows though :-(
Patch Set #13, Line 300:
err = windows.NtCreateNamedPipeFile(&h, access, &oa, &iosb, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, disposition, 0, typ, 0, 0, 0xffffffff, uint32(c.InputBufferSize), uint32(c.OutputBufferSize), &timeout)I'm not a fan of using native NT APIs unless absolutely necessary. Are we aware of any particular reason why we are using NtCreateNamedPipeFile here, instead of CreateNamedPipe?
By invoking the latter, we probably can completely avoid the above ACL stuff; by passing in nil security attributes, the API will do all of that default ACL work for us. Plus we'd be using an API that is actually officially supported! :-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
4 comments:
File windows/namedpipe/file.go:
Patch Set #13, Line 73:
_, err := windows.CreateIoCompletionPort(h, ioCompletionPort, 0, 0)I think it would be worthwhile to specify a non-zero completion key. […]
This suggestion is harmless but seems unnecessary. Such a bad actor could easily wreak havoc in myriad other ways... there doesn't seem to be a lot of benefit.
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
GetQueuedCompletionStatus associates the current OS thread with the IOCP. […]
I wonder if it would be better to set a very large maximum concurrency count so that you don't have to do this... it may give the Go scheduler more flexibility to avoid context switches if you can skip this.
Patch Set #13, Line 132:
panic(err)
I am not sure we should unconditionally panic in this case. […]
Again, it seems unnecessary to protect against this case. Indeed, it seems strictly worse to ignore unknown keys, since the presence of them is a clear sign that something in the process is FUBAR, and it's better to panic when you detect that (and allow the developer to fix the bug) then pretend that nothing happened.
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)
I don't think that this code is necessary. […]
That's not the case. WSAGetOverlappedResult translates certain Windows errors to WSAE* errors for compatibility with BSD socket APIs.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 300:
err = windows.NtCreateNamedPipeFile(&h, access, &oa, &iosb, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, disposition, 0, typ, 0, 0, 0xffffffff, uint32(c.InputBufferSize), uint32(c.OutputBufferSize), &timeout)I'm not a fan of using native NT APIs unless absolutely necessary. […]
The comment above gives a hint to this: "By not asking for read or write access, the named pipe file system will put this pipe into an initially disconnected state, blocking client connections until the next call with isFirstPipe == false".
This functionality is not exposed via CreateNamedPipe.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 300:
err = windows.NtCreateNamedPipeFile(&h, access, &oa, &iosb, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, disposition, 0, typ, 0, 0, 0xffffffff, uint32(c.InputBufferSize), uint32(c.OutputBufferSize), &timeout)
The comment above gives a hint to this: "By not asking for read or write access, the named pipe file […]
(Probably a clarifying comment is in order.)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
2 comments:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {I wonder if it would be better to set a very large maximum concurrency count so that you don't have […]
From the perspective of the Go scheduler, you are correct. However, from the perspective of the OS scheduler and broader Win32 implications, it might be troublesome.
IOCPs are essentially kernel-mode, thread-safe queues that are in cahoots with the OS scheduler and the I/O subsystem.
An IOCP's concurrency count is essentially, "how many OS threads may concurrently process packets obtained via GetQueuedCompletionStatus?" If the concurrency count is N, and N+1 threads call GetQueuedCompletionStatus, and assuming that there are enough incoming completion packets to satisfy all N requests, 1 thread will remain blocked.
But the reason why GetQueuedCompletionStatus has an association with the OS thread is because, if an associated thread goes idle, the scheduler notifies the I/O subsystem that it should release another thread that is blocked on GetQueuedCompletionStatus for the same IOCP. In the above hypothetical example, that 1 thread will then be unblocked and provided with a completion packet.
As long as we only have a single goroutine processing requests, then it doesn't really make sense for the kernel to be passing idle notifications to the IOCP to unblock other threads; no matter where the go scheduler executes that goroutine, only one GetQueuedCompletionStatus call will ever be executed concurrently.
Furthermore (and perhaps most importantly), OS threads may only be associated with a single IOCP at a time. In other words, if this package's IOCP is not the only one in the process and we leave everything up to the go scheduler, we could end up with "thrashing" in the sense that OS threads could be constantly re-associating themselves with different IOCPs at different times. At that point, I'm not sure it's even beneficial to use IOCPs anymore; you might as well be using events.
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)
That's not the case. […]
Look again: the actual values of those WSAE* errors are 1:1 with Win32.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
From the perspective of the Go scheduler, you are correct. […]
Maybe that's a trade-off worth making, but then it's probably worth measuring the effects.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
Maybe that's a trade-off worth making, but then it's probably worth measuring the effects.
Oh, and lest my "thrashing" comment be viewed as theoretical: I have seen thread disassociation with IOCP trigger outright deadlocks in the past.
https://ohmsblog.teamohms.org/2009/05/leaky-abstractions-redux-part-ii.html
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)Look again: the actual values of those WSAE* errors are 1:1 with Win32.
Concretely, STATUS_TIMEOUT maps to ERROR_TIMEOUT for Win32 APIs, but WSAETIMEDOUT for Winsock. ERROR_TIMEOUT and WSAETIMEDOUT have different values. WSAGetOverlappedResult is the function that implements this special case, along with others.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Aaron Klotz, Simon Rozman.
8 comments:
Patchset:
Thanks for your feedback, Aaron. I didn't find the interesting discussion actionable for this CL, but I do look forward to seeing the performance-related one after this lands, if you feel like doing that.
File windows/namedpipe/file.go:
Let's add: […]
Nak.
Patch Set #13, Line 41:
h, err := windows.CreateIoCompletionPort(windows.InvalidHandle, 0, 0, 0)
If we're only going to start one ioCompletionProcessor goroutine, then we should set 1 as the final […]
Nak.
Patch Set #13, Line 45:
ioCompletionPort = hSet ioCompletionKey to a (preferably cryptographically safe) random number.
Nak.
Patch Set #13, Line 73:
_, err := windows.CreateIoCompletionPort(h, ioCompletionPort, 0, 0)
This suggestion is harmless but seems unnecessary. […]
If the rationale for this is "protecting against DLL injection", then I must decline. As John wrote, there are lots of ways to fool that. I'm not going to add a bunch of halfbaked "mitigations" that don't do anything. No part of Go's libraries have ever done that, and most times I've seen these sorts of things in the wild, I've had no trouble working around this, and my exploits still work just fine.
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
Oh, and lest my "thrashing" comment be viewed as theoretical: I have seen thread disassociation with […]
I haven't observed any thread thrashing with this even in very busy processes. Go's scheduler generally doesn't move things around when there's little reason to do so. So I'm not sure this is worth locking a thread over.
What about this: after this lands, you can submit a followup CL that shows the performance delta and contains the usual reasoning for making performance-related changes like that. Sound okay? If the numbers look good, I'll happily +2 it. If they don't, then at least something interesting will have been learned by the exercise.
Patch Set #13, Line 132:
panic(err)
Again, it seems unnecessary to protect against this case. […]
Agree with John here for the reasons I mentioned above. Nak'ing this.
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)
Concretely, STATUS_TIMEOUT maps to ERROR_TIMEOUT for Win32 APIs, but WSAETIMEDOUT for Winsock. […]
John is correct.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Aaron Klotz, Simon Rozman.
4 comments:
Patchset:
Ooops, missed these. Conclusion is same, though; won't be posting a v+1 in response.
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 254:
defer windows.LocalFree(windows.Handle(unsafe.Pointer(ntPath.Buffer)))HeapFree(GetProcessHeap(), 0, unsafe.Pointer(ntPath. […]
Addressed below.
Patch Set #13, Line 267:
defer windows.LocalFree(windows.Handle(unsafe.Pointer(acl)))
HeapFree(GetProcessHeap(), 0, unsafe.Pointer(acl)) is the more appropriate API to use here IMHO. […]
Generally with Win32 usage throughout Go, we follow MSDN's suggestion, where if something is LocalAlloc'd, it's also LocalFree'd. We rarely have use for the more general help functions, since Go itself has an alloc, so we're just placating various Win32 API requirements. (And I have no intention of adding the heap functions to x/sys either.)
Patch Set #13, Line 300:
err = windows.NtCreateNamedPipeFile(&h, access, &oa, &iosb, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, disposition, 0, typ, 0, 0, 0xffffffff, uint32(c.InputBufferSize), uint32(c.OutputBufferSize), &timeout)
(Probably a clarifying comment is in order. […]
Indeed John is correct. Popping CreateNamedPipe into IDA indicates there's no path to getting the same thing.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {I haven't observed any thread thrashing with this even in very busy processes. […]
Btw, the ideal is that we integrate into the IOCP that the netpoller is already using rather than have our own. But that's not exposed via runtime at the moment.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, John Starks, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
Btw, the ideal is that we integrate into the IOCP that the netpoller is already using rather than ha […]
Hah, funny, I was just looking into that. I was thinking some sort of "escape hatch" API only accessible via go:linkname, with semantics for adding a callback for a given completion key, and then segment the bits of completion key in half, with the runtime getting half of them and this escape hatch getting the other.
Is that something along the lines of what you were thinking? Or were you thinking something even more rudimentary where you can just register a set of functions to be called whenever netpoll is called?
Maybe I'll write up a Go proposal for this.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)John is correct.
Hmm, yeah I reviewed my bible for this (Network Programming for Microsoft Windows, 2ed by Anthony Jones and Jim Ohlund).
For posterity: it depends the context under which you are consuming the error. If the consumer of the error code has no need for BSD socket compatibility, then it doesn't matter (which has been my experience writing IOCP-based I/O services in the past, hence my previous comments).
But if the consumer *does* require BSD compat, then yes, this is necessary.
Apologies.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz, John Starks, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 167:
err = windows.WSAGetOverlappedResult(f.handle, &c.o, &bytes, false, &flags)
Hmm, yeah I reviewed my bible for this (Network Programming for Microsoft Windows, 2ed by Anthony Jo […]
Technically, as Alex pointed out, this branch is dead code anyway, since f.socket is always false. But if we ever pull in anything else from John's go-winio, this will prove handy, and I think it's a good abstraction to keep around.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {Hah, funny, I was just looking into that. […]
Ah, but who gets to own the escape hatch? x/sys? Is there precedent for a single privileged callout like this? What if go-winio wants to take the slot for supporting wait completion packets, as an experiment before we promote the code to x/sys?
There's also the question of whether it's safe to allow arbitrary code to run on the netpoll thread...
I'm wondering if it would be sufficient to essentially just have a model for getting the response on a channel, like we do here. To be sufficiently generic, we'd have to have a convention for getting the channel reference from the overlapped pointer (which raises some questions about GC and reachability...).
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Klotz (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {Hah, funny, I was just looking into that. […]
I am still concerned about deadlocks.
My experience as outlined in my blog post above is that (at least given the state of the NT kernel at time that blog post was written), a thread that is disassociating from the IOCP does *not* necessarily notify its previous IOCP that this is occurring. Subsequent calls to GetQueuedCompletionStatus just block because, as far as its port is concerned, its concurrency count is still satisfied.
Hmmm... maybe John's suggestion of a ridonculously (that's a real word) large concurrency count is the only way to prevent that.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
I am still concerned about deadlocks. […]
That's an interesting point. Let me check the kernel source (I work on the Windows team at MS) to see if that's still possible in modern versions of Windows. A lot has changed since the Vista era.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 125:
func ioCompletionProcessor(h windows.Handle) {
That's an interesting point. […]
From code inspection, it does not appear that this is possible--when the kernel disassociates a thread from a queue, it decrements that queue's active thread count and wakes up a new thread if one was previously blocked due to the concurrency limit.
It appears that this has been the intent as far back as I can find (Windows 2000)... but it's certainly possible that there was a subtle bug somewhere. If so, it seems unlikely to me that this bug persists in modern versions of Windows given how the code has evolved and how extensively we use IOCPs in the operating system now.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
Patchset:
Can this be +2'd please?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Aaron Klotz, Simon Rozman.
1 comment:
Patchset:
Can this be +2'd please?
It doesn't sound like there's consensus and I'm not qualified to review this myself.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
Patchset:
It doesn't sound like there's consensus and I'm not qualified to review this myself.
I'm not aware of any unresolved disagreements.
Aaron - was there something more you wanted to add that I overlooked?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
1 comment:
Patchset:
I'm not aware of any unresolved disagreements. […]
Aaron/John - if this seems fine to you, can you +1? I get the idea Brad is waiting for some outside assessment.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz, Simon Rozman.
Patch set 13:Code-Review +1
Simon Rozman (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Aaron Klotz.
Patch set 13:Code-Review +1
1 comment:
Patchset:
Cool, LGTM
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz.
1 comment:
Patchset:
Brad - could you let me know whether you require anything else here?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Aaron Klotz.
1 comment:
Patchset:
Pinging again here, in case somebody wants to submit this reviewed CL...
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
Is there a proposal issue for this new API?
This is x/sys, not stdlib. Please remove your -2.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
API changes in x repos should be reviewed as proposals. […]
That has not been the case with any of the system calls I've added to x/sys/windows, ever.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
That's unfortunate. Please use the proposal process for future API changes. […]
That's a bit too much overhead for me, sorry. Especially considering I have no voice on the proposal's committee, despite being the primary committer to x/sys/windows (according to github stats). I think I'll save more time porting my stuff to a different language and runtime, rather than going through cycles of maintainer neglect and then getting drowned in bureaucracy and private processes. I just can't see how this is a good use of time.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
That's a bit too much overhead for me, sorry. […]
zx2c4@thinkpad ~/Projects/golang-dev/sys $ git log windows | sed -n 's^.*golang/go#\([0-9]\+\).*^https://github.com/golang/go/issues/\1^p' | xargs chromium
I just ctrl+tab'd through every single issue referenced on every single x/sys/windows commit since the beginning of time. The only two that are proposals are for std's syscall, which then had a component in x/sys/windows (implemented by me). That means there is not a single proposal for x/sys/windows. Is my sed regex wrong?
It really feels like a different standard is being applied here than normal. Have policies changed in the last week or two? In which case, can I unironically ask "was there a proposal for that?" without evoking ire?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Nishanth Shanmugham (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
If somehow code changes in a way that err is defined on line 258 ...
Worth clarifying/noting that even written as it is (regardless of whether err is defined before line 258), a := assignment will always create a local sd variable and not update the sd parameter.
See https://play.golang.org/p/qkVBAxEHdCc for an example.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
zx2c4@thinkpad ~/Projects/golang-dev/sys $ git log windows | sed -n 's^.*golang/go#\([0-9]\+\). […]
We have historically permitted direct translations of system calls/OS functions in x/sys/windows and x/sys/unix without a proposal. But we've also used the proposal mechanism for more complex changes, such as https://golang.org/issue/46084 or https://golang.org/issue/24918 or https://golang.org/issue/24843.
I agree that as far as I know there have not been any accepted proposals for x/sys/windows. But as far as I know there also haven't been any changes that go beyond a simple Go wrapper for an OS funtion.
This CL and https://golang.org/cl/359054 are adding new package, not just new simple system calls, so it seems appropriate for them to go through the proposal process. I don't think this is a different standard. I think it's a reflection of the scale of the changes.
I'm sorry that we are throwing roadblocks at this. I actually think the proposal process will help these CLs because it will give people some confidence that the APIs are reasonable.
I would be happy to file the proposals for these two CLs for you if that would help.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
Thank you for responding.
I have no trouble with playing by the rules, provided they're fair, sensible, and won't waste more time than trying to get Windows reviews already has.
I agree that as far as I know there have not been any accepted proposals for x/sys/windows.
Yet there are already several subpackages of x/sys/windows, and countless changes that are _not_ a boring direct translation of system calls. I have made many of those changes. Others have made many of those changes. It's hard to not view this as a sudden change in policy.
I actually think the proposal process will help these CLs because it will give people some confidence that the APIs are reasonable.
Actually, every maintainer I spoke to said that while they were equipped to comment on the API and "Go things", they didn't possess the expertise to review Windows code. Read on for why this matters.
I'm sorry that we are throwing roadblocks at this.
It's not as though it's a particularly large roadblock, but it comes on the heals of something else that I think can't be separated from it: maintainer neglect and a lack of care for Windows reviews and maintenance. I tried for weeks to get a review of this CL and the other one Russ made the same comment on. Crickets. Every maintainer I spoke to personally said, "I have no idea about Windows stuff, find somebody else." Nobody appeared. At the same time, the windows/arm32 unbitrotting work I did is now bitrotting again, as the Go team has failed to set up a builder for it after giving me the go ahead to take down mine. Neglect.
Now I don't think this is a matter of anybody's bad intent. There's quite simply not a lot of interest or headcount in doing Windows things. How many Go maintainers run Windows as their primary dev machine daily? While many have expertise and a vested stake in darwin and linux working well, the same can't be said about Windows. Again, it's not like this is some kind of sin -- it's a weird platform, after all, and I myself write to you as a Linux kernel developer, not a Windows guy -- but it is the simple state affairs. The result: nobody's eager to do jump in on this.
So naturally I start to wonder, "if this is an afterthought for the development team, is it reasonable for me to be spending time on it and writing my own code to depend on it? Maybe I should move to a different platform." I think that's a reasonable reflection from a developer point of view.
And then.
And then, rather than that long-awaited review finally coming, a procedural technicality is invoked, and I'm back to another round of wasting time or waiting or both. It reminds me of bureaucracies that don't want to process your papers, so they just send you in circles long enough for you to give up. Paperwork as a means of deferring real work: it's a real thing and it happens everywhere.
So, my initial wondering has now expanded. Not only are reviews of this stuff an afterthought, but they also come attached to more bureaucratic overhead, involving closed door validation from folks who already don't possess either the time to or the ability to review Windows things. I wonder again: is *this* a reasonable use of my time?
So, that's where I am. Again, I have no problem following procedures that are fair and sensible; you've seen me make proposals before and carry things out according to the book. But it needs to make sense in the grand scheme of things. And in this case here, the puzzle pieces don't all fit together.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
Thank you for responding. […]
[Setting thread to "Unresolved".]
Patrik Nyblom (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
[Setting thread to "Unresolved". […]
I can't answer everything here, but the fact that I'm designated to handle Windows things is a step to address the "I don't know this stuff, ask someone else" problem. Basically, I am "someone else", even if that is kind of a new development (other responsibilities, gradual transfer, etc etc).
That does not mean I can promise to always be able to review things in a certain time frame, and when this landed on my table, we had a fair amount of Windows related release blockers that I prioritized, coupled with the fact that I'm new to the runtime and maybe not the fastest (yet, or will ever be, that's indeterminate).
So, what I'm saying is - regarding Windows reviews, the problem is not ignored.
Regarding policy questions, I'm not the person to answer. I just this morning submitted my first cl in Gerrit.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
I can't answer everything here, but the fact that I'm designated to handle Windows things is a step […]
I promise that we are not suggesting the proposal process as a procedural technicality as a way to defer actual work.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
> Yet there are already several subpackages of x/sys/windows, and countless changes that are _not_ a […]
Thanks for your response and for the discussion yesterday. I think we're on the same page now. I look forward to Patrik's involvement. I'll mark this thread as resolved and follow up with proposal particulars on Github.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/file.go:
Patch Set #13, Line 41:
h, err := windows.CreateIoCompletionPort(windows.InvalidHandle, 0, 0, 0)Nak.
Sorry, thought this was part of the "use a random number" suggestions and missed it before. I'll set the threads param here for the next revision.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Jason A. Donenfeld uploaded patch set #14 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Fixes golang/go#49650.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,447 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Patch set 14:Trust +1
1 comment:
Patchset:
Updated with latest proposal changes and threadCount param == 1 fix from Aaron.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Jason A. Donenfeld uploaded patch set #15 to this change.
windows/namedpipe: add simple named pipe library
This new library adds an often wanted simple feature of listening on
named pipes and connecting to named pipes.
It is based on WireGuard's winpipe library, which in turn is based on
parts of Microsoft's go-winio library.
Fixes golang/go#49650.
Change-Id: I382a5e17d5524ea76a19f014d6ff15e7b01e5017
---
A windows/namedpipe/namedpipe_test.go
A windows/namedpipe/file.go
A windows/namedpipe/namedpipe.go
3 files changed, 1,447 insertions(+), 0 deletions(-)
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Patch set 15:Run-TryBot +1Trust +1
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Jason A. Donenfeld removed a vote from this change.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Patch set 15:Code-Review -2
1 comment:
Patchset:
I definitely did not mean to just remove that -2. -2ing it myself until the proposal is approved.
Patrik Nyblom (Gerrit)
Attention is currently required from: Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
2 comments:
Patchset:
Pre-reviewing a bit, while waiting for the proposal process.
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 419:
// The first connection is swallowed on Windows 7 & 8I'm curious, what is this? Could you elaborate?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 419:
// The first connection is swallowed on Windows 7 & 8I'm curious, what is this? Could you elaborate?
On Windows 7 and 8, the first time a client uses Dial(), the Dial() will never complete (and eventually timeout if a context.WithTimeout is used). However, subsequent ones work just fine. I assume that the non-standard specific use of NtCreateNamedPipeFile (which we're using so that this work with Go's design) triggers a bug in those old kernels, and because those versions are now ancient, we work around it. The workaround here is pretty basic - we just Dial the pipe ourselves immediately after creating, so that the next Dial from a real client works.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Patrik Nyblom (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #13, Line 419:
// The first connection is swallowed on Windows 7 & 8
On Windows 7 and 8, the first time a client uses Dial(), the Dial() will never complete (and eventua […]
hah, I'd never heard of that bug, but that doesn't mean much - there's much I haven't heard of:) The NtCreateNamedPipeFile theory seems reasonable, and the workaround is fine with me, I was just curious.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Patrik Nyblom (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #15, Line 2:
Copyright 2015 Microsoftgo-winio has a MIT license, meaning that copyright notice and permission notice needs to be included with software that uses this (? - I'm not a lawyer).
Is there a way we can get Microsoft to "donate" the parts used? Maybe @jostarks has some thoughts?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #15, Line 2:
Copyright 2015 Microsoft
Is there a way we can get Microsoft to "donate" the parts used
Sounds like months of lawyers...
go-winio has a MIT license, meaning that copyright notice and permission notice needs to be included with software that uses this (? - I'm not a lawyer).
Go uses MIT as well. The copyright notice is retained in the file headers.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, John Starks, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #15, Line 2:
Copyright 2015 Microsoft
> Is there a way we can get Microsoft to "donate" the parts used […]
If it isn't easy, we can think of something else.
The difference is that people using x/sys/windows are already required to incorporate the Go license in their distribution. That is required by anybody who distributes any Go program.
If we add the Microsoft license, then they are *also* required to include the Microsoft license. This is based on looking at the LICENSE file for https://github.com/microsoft/go-winio; if that is not right, please let me know. Note that that license file is not the same as the Go license file--for one thing, it's copyright Microsoft. Just adding a copyright notice to the source code does not satisfy the requirements that that license imposes on people who distribute software.
Requiring the mention of another license in their distributions is a hassle for everyone who distributes software that uses x/sys/windows. It's the kind of hassle we want to avoid imposing on people if there is any way that we can avoid it.
So while dealing this now is itself a hassle for you and us, it's the best way forward for the overall ecosystem.
Alternatively, perhaps it is possible to write this code such that it is not derivative of the Microsoft code, and then we can simply omit the Microsoft copyright.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason A. Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, John Starks, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #15, Line 2:
Copyright 2015 Microsoft
If it isn't easy, we can think of something else. […]
Hm, rough situation potentially. I have no idea how to proceed here. I'll continue reflecting changes from the proposal to this CL and addressing technical concerns that are brought up. But I suspect a Googler who knows the ropes with this legal stuff ought to take the lead on figuring out the other parts.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
John Starks (Gerrit)
Attention is currently required from: Jason A. Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
File windows/namedpipe/namedpipe.go:
Patch Set #15, Line 2:
Copyright 2015 Microsoft
Hm, rough situation potentially. I have no idea how to proceed here. […]
I will try to follow up with our legal team, but it may take a little while.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason Donenfeld (Gerrit)
Attention is currently required from: Jason Donenfeld, Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
Jason Donenfeld removed a vote from this change.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
The proposal for this was accepted, so removing the -2. Hopefully we can get this submitted at long last.
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason Donenfeld (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Brad Fitzpatrick, Ian Lance Taylor, Aaron Klotz, Russ Cox.
1 comment:
Patchset:
Looks like I first submitted this March 4, 2021, so we've passed the year mark, and for the last several months it's been basically ready. I think it's time we merge this?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Jason Donenfeld (Gerrit)
Attention is currently required from: Aaron Klotz, Alex Brainman, Brad Fitzpatrick, Ian Lance Taylor, Patrik Nyblom, Russ Cox.
1 comment:
Patchset:
Looks like I first submitted this March 4, 2021, so we've passed the year mark, and for the last sev […]
Can we submit this finally?
To view, visit change 299009. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Bryan Mills added 1 comment![]( "Open in Gerrit")
Looks like I first submitted this March 4, 2021, so we've passed the year mark, and for the last several months it's been basically ready. I think it's time we merge this?
Can we submit this finally?
The last update to the issue was https://go.dev/issue/49650#issuecomment-1311959888, so it appears this is still awaiting a resolution to the licensing question.
Related details
- Aaron Klotz
- Alex Brainman
- Brad Fitzpatrick
- Ian Lance Taylor
- Jason Donenfeld
- Patrik Nyblom
- Russ Cox
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |