utils.go functions to not require os.File

[antonio-osorio]

Would you be open to modify the signature of the functions in utils.go:

• InheritSize
• SetSize
• GetSizeFull
• GetSize

to either accept a File-like interface:

type FileLike interface {
	Fd() uintptr
}

or to just require the the File descriptor myFile.Fd() instead of the os.File concrete struct?

func InheritSize(ptyFd, ttyFd uintptr) error {
...
}

If I am not mistaken otherwise it requires the caller to unnecessarily create a os.File object:

var myFileLike FileLike
myFile := os.NewFile(myFileLike.Fd(), "")
pty.InheritSize(myFile, myOtherFile)

[creack]

In what situation do you have a fd without os.File? I had that situation when dealing with CGO, but it is quite specific and rare.
As those Size functions can only work with a terminal fd like the one pty.Start returns, I am concerned that modifying the signature to an interface would be more confusing than helpful in most cases.

Could you share more detail about your use case?

FYI, I have another lib https://github.com/creack/termios which is lower level and more "generic" expecting fds directly

[antonio-osorio]

The key use case is that I am running into is when using the inherit size:

pty.InheritSize(context.Stdout, ptmx)

The ptmx was created by this library so it meets the type constraint. But sometimes (i.e. within a test runner) context.Stdout gets replaced by a file like object that would do more complex routing/printing/copying/etc.

[creack]

Thank you for the detail, but this seems like a very specific case, and I don't think it would justify a change in the public api / major version bump. As you mentioned, os.NewFile does the trick and I believe switching to FDs directly would confuse users.

Note that a "file-like" object which is not backed by an actual tty/pty will fail those calls with "invalid ioctl for device".
Also note that InheritSize is meant to set the pty to the size of the parent terminal, i.e. os.Stdin & co. If the process is in a test runner context, those would be the tty of the runner.

Closing for now. Let me know if you have more questions.

[antonio-osorio]

An issue that I just ran with using the FD to create the new os.File object is that when the function with the os.File ends the os.File goes out of scope and gets gc'ed, causing the FD to be closed.

[creack]

This is indeed a good use case. I don't think it is wide enough to warrant a breaking change in the api, but would an additional helper work for you?

[kr]

An issue that I just ran with using the FD to create the new os.File object is that when the function with the os.File ends the os.File goes out of scope and gets gc'ed, causing the FD to be closed.

The File methods in package net dup the fd for this reason. e.g. https://pkg.go.dev/net#TCPConn.File

The purpose of TCPConn.File is to give you access to the fd for a TCP connection, so you can do ioctl stuff etc. It would be bad if closing the os.File also closed the TCP connection, so the returned os.File uses a dup of the fd. This pty issue seems like an analogous situation, and I think the same workaround might be appropriate. This would mean there's still no need for additional API surface in creack/pty.