Review feedback.

jterry75 · jterry75 · commit af1b6a026eed · 2018-07-27T09:49:33.000-07:00
1. Moves the log message for each socket to the appropriate _unix and
_windows.go
2. Replaces all reference to Abstract Socket for Windows.
3. Adds support for ctrl+c on Windows to exit a shim.

Signed-off-by: Justin Terry (VM) &lt;juterry@microsoft.com&gt;
diff --git a/runtime/v2/shim/shim.go b/runtime/v2/shim/shim.go
@@ -202,11 +202,10 @@ func (s *Client) Serve() error {
 // serve serves the ttrpc API over a unix socket at the provided path
 // this function does not block
 func serve(ctx context.Context, server *ttrpc.Server, path string) error {
-	l, path, err := serveListener(path)
+	l, err := serveListener(path)
 	if err != nil {
 		return err
 	}
-	logrus.WithField("socket", path).Debug("serving api on abstract socket")
 	go func() {
 		defer l.Close()
 		if err := server.Serve(ctx, l); err != nil &&
diff --git a/runtime/v2/shim/shim_unix.go b/runtime/v2/shim/shim_unix.go
@@ -47,7 +47,7 @@ func setupDumpStacks(dump chan<- os.Signal) {
 	signal.Notify(dump, syscall.SIGUSR1)
 }
 
-func serveListener(path string) (net.Listener, string, error) {
+func serveListener(path string) (net.Listener, error) {
 	var (
 		l   net.Listener
 		err error
@@ -57,14 +57,15 @@ func serveListener(path string) (net.Listener, string, error) {
 		path = "[inherited from parent]"
 	} else {
 		if len(path) > 106 {
-			return nil, path, errors.Errorf("%q: unix socket path too long (> 106)", path)
+			return nil, errors.Errorf("%q: unix socket path too long (> 106)", path)
 		}
 		l, err = net.Listen("unix", "\x00"+path)
 	}
 	if err != nil {
-		return nil, path, err
+		return nil, err
 	}
-	return l, path, nil
+	logrus.WithField("socket", path).Debug("serving api on abstract socket")
+	return l, nil
 }
 
 func handleSignals(logger *logrus.Entry, signals chan os.Signal) error {
diff --git a/runtime/v2/shim/shim_windows.go b/runtime/v2/shim/shim_windows.go
@@ -54,20 +54,29 @@ func setupDumpStacks(dump chan<- os.Signal) {
 
 // serve serves the ttrpc API over a unix socket at the provided path
 // this function does not block
-func serveListener(path string) (net.Listener, string, error) {
+func serveListener(path string) (net.Listener, error) {
 	if path == "" {
-		return nil, path, errors.New("'socket' must be npipe path")
+		return nil, errors.New("'socket' must be npipe path")
 	}
 	l, err := winio.ListenPipe(path, nil)
 	if err != nil {
-		return nil, path, err
+		return nil, err
 	}
-	return l, path, nil
+	logrus.WithField("socket", path).Debug("serving api on npipe socket")
+	return l, nil
 }
 
 func handleSignals(logger *logrus.Entry, signals chan os.Signal) error {
-	<-signals
-	return nil
+	logger.Info("starting signal loop")
+	for {
+		select {
+		case s := <-signals:
+			switch s {
+			case os.Interrupt:
+				break
+			}
+		}
+	}
 }
 
 func (l *remoteEventsPublisher) Publish(ctx context.Context, topic string, event events.Event) error {
diff --git a/runtime/v2/shim/util_windows.go b/runtime/v2/shim/util_windows.go
@@ -37,7 +37,7 @@ func SetScore(pid int) error {
 	return nil
 }
 
-// SocketAddress returns an abstract npipe address
+// SocketAddress returns a npipe address
 func SocketAddress(ctx context.Context, id string) (string, error) {
 	ns, err := namespaces.NamespaceRequired(ctx)
 	if err != nil {
@@ -46,7 +46,7 @@ func SocketAddress(ctx context.Context, id string) (string, error) {
 	return fmt.Sprintf("\\\\.\\pipe\\containerd-shim-%s-%s-pipe", ns, id), nil
 }
 
-// AnonDialer returns a dialer for an abstract npipe
+// AnonDialer returns a dialer for a npipe
 func AnonDialer(address string, timeout time.Duration) (net.Conn, error) {
 	return winio.DialPipe(address, &timeout)
 }
@@ -55,7 +55,7 @@ func AnonDialer(address string, timeout time.Duration) (net.Conn, error) {
 func NewSocket(address string) (net.Listener, error) {
 	l, err := winio.ListenPipe(address, nil)
 	if err != nil {
-		return nil, errors.Wrapf(err, "failed to listen to abstract npipe %s", address)
+		return nil, errors.Wrapf(err, "failed to listen to npipe %s", address)
 	}
 	return l, nil
 }

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ func setupDumpStacks(dump chan<- os.Signal) {`
`47`	`47`	`signal.Notify(dump, syscall.SIGUSR1)`
`48`	`48`	`}`
`49`	`49`
`50`		`-func serveListener(path string) (net.Listener, string, error) {`
	`50`	`+func serveListener(path string) (net.Listener, error) {`
`51`	`51`	`var (`
`52`	`52`	`l net.Listener`
`53`	`53`	`err error`
`@@ -57,14 +57,15 @@ func serveListener(path string) (net.Listener, string, error) {`
`57`	`57`	`path = "[inherited from parent]"`
`58`	`58`	`} else {`
`59`	`59`	`if len(path) > 106 {`
`60`		`- return nil, path, errors.Errorf("%q: unix socket path too long (> 106)", path)`
	`60`	`+ return nil, errors.Errorf("%q: unix socket path too long (> 106)", path)`
`61`	`61`	`}`
`62`	`62`	`l, err = net.Listen("unix", "\x00"+path)`
`63`	`63`	`}`
`64`	`64`	`if err != nil {`
`65`		`- return nil, path, err`
	`65`	`+ return nil, err`
`66`	`66`	`}`
`67`		`- return l, path, nil`
	`67`	`+ logrus.WithField("socket", path).Debug("serving api on abstract socket")`
	`68`	`+ return l, nil`
`68`	`69`	`}`
`69`	`70`
`70`	`71`	`func handleSignals(logger *logrus.Entry, signals chan os.Signal) error {`
Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ func SetScore(pid int) error {`
`37`	`37`	`return nil`
`38`	`38`	`}`
`39`	`39`
`40`		`-// SocketAddress returns an abstract npipe address`
	`40`	`+// SocketAddress returns a npipe address`
`41`	`41`	`func SocketAddress(ctx context.Context, id string) (string, error) {`
`42`	`42`	`ns, err := namespaces.NamespaceRequired(ctx)`
`43`	`43`	`if err != nil {`
`@@ -46,7 +46,7 @@ func SocketAddress(ctx context.Context, id string) (string, error) {`
`46`	`46`	`return fmt.Sprintf("\\\\.\\pipe\\containerd-shim-%s-%s-pipe", ns, id), nil`
`47`	`47`	`}`
`48`	`48`
`49`		`-// AnonDialer returns a dialer for an abstract npipe`
	`49`	`+// AnonDialer returns a dialer for a npipe`
`50`	`50`	`func AnonDialer(address string, timeout time.Duration) (net.Conn, error) {`
`51`	`51`	`return winio.DialPipe(address, &timeout)`
`52`	`52`	`}`
`@@ -55,7 +55,7 @@ func AnonDialer(address string, timeout time.Duration) (net.Conn, error) {`
`55`	`55`	`func NewSocket(address string) (net.Listener, error) {`
`56`	`56`	`l, err := winio.ListenPipe(address, nil)`
`57`	`57`	`if err != nil {`
`58`		`- return nil, errors.Wrapf(err, "failed to listen to abstract npipe %s", address)`
	`58`	`+ return nil, errors.Wrapf(err, "failed to listen to npipe %s", address)`
`59`	`59`	`}`
`60`	`60`	`return l, nil`
`61`	`61`	`}`