Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zos: share implementation with unix (less DRY) #63

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

thaJeztah
Copy link
Member

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
Copy link
Member Author

@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 thaJeztah marked this pull request as ready for review February 14, 2022 16:56
Copy link
Contributor

@najohnsn najohnsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bringing this to my attention! Looks great!
Testing on z/OS passed.

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 {

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

Rebased (after #62 was merged)

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

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@samuelkarp samuelkarp merged commit f6c071f into containerd:main Oct 7, 2022
@thaJeztah thaJeztah deleted the unify_zos branch October 7, 2022 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants