console icon indicating copy to clipboard operation
console copied to clipboard

zos: share implementation with unix (less DRY)

Open thaJeztah opened this issue 3 years ago • 2 comments

zos: share implementation with unix

The zos and "unix" implementations were identical, except for the NewPTY() function.

This patch extracts that function to non-exported implementations for "zos" and "unix", so that all other bits can be shared.

For reference; this was the diff between both files before:

diff --git a/console_unix.go b/console_zos.go
index 78f70c2..96ccb6f 100644
--- a/console_unix.go
+++ b/console_zos.go
@@ -17,6 +17,9 @@
 package console

 import (
+	"fmt"
+	"os"
+
    "golang.org/x/sys/unix"
 )

@@ -24,16 +27,20 @@ import (
 // The master is returned as the first console and a string
 // with the path to the pty slave is returned as the second
 func NewPty() (Console, string, error) {
-	f, err := openpt()
-	if err != nil {
-		return nil, "", err
-	}
-	slave, err := ptsname(f)
-	if err != nil {
-		return nil, "", err
-	}
-	if err := unlockpt(f); err != nil {
-		return nil, "", err
+	var f File
+	var err error
+	var slave string
+	for i := 0; ; i++ {
+		ptyp := fmt.Sprintf("/dev/ptyp%04d", i)
+		f, err = os.OpenFile(ptyp, os.O_RDWR, 0600)
+		if err == nil {
+			slave = fmt.Sprintf("/dev/ttyp%04d", i)
+			break
+		}
+		if os.IsNotExist(err) {
+			return nil, "", err
+		}
+		// else probably Resource Busy
    }
    m, err := newMaster(f)
    if err != nil {

zos: further reconcile implementation with other unices

Follow-up to the previous commit; instead of using separate implementations for newPty() as a whole, create implementations for openpt(), ptsname() and a stub for unlockpt().

This seems slightly more consistent with other implementations, which already had os/arch specific implementations for these functions.

thaJeztah avatar Feb 14 '22 09:02 thaJeztah

@estesp @najohnsn PTAL if this makes sense to do.

I noticed that the zos and _unix.go implementations only differed in NewPTY(), so thought to split out just that implementation (first commit). When doing so, I noticed that for other implementations, we used specialised functions for openpt(), ptsname() and unlockpt(), so I gave that a try in the second commit (perhaps that approach is more consistent?).

Happy to either drop the last commit (if the first implementation is clearer), or to squash both commits. Or of course to just close this PR if we don't think it's worth the effort 😅

thaJeztah avatar Feb 14 '22 09:02 thaJeztah

Rebased (after https://github.com/containerd/console/pull/62 was merged)

@mxpv @estesp ptal (happy to squash the commits if you prefer)

thaJeztah avatar Mar 21 '22 18:03 thaJeztah