Skip to content

Use unix.Ioctl{Get,Set}Termios on all unix platforms#16

Merged
cpuguy83 merged 2 commits intomoby:masterfrom
tklauser:unix-termios
Sep 11, 2020
Merged

Use unix.Ioctl{Get,Set}Termios on all unix platforms#16
cpuguy83 merged 2 commits intomoby:masterfrom
tklauser:unix-termios

Conversation

@tklauser
Copy link
Contributor

@tklauser tklauser commented Sep 3, 2020

Unify the implementation for all unix platforms to use IoctlGetTermios
and IoctlSetTermios from the golang.org/x/sys/unix package instead of
implementing it in terms of SYS_IOCTL. This avoids duplicating code,
makes error handling easier and fixes the build on darwin where direct
syscalls, i.e. unix.Syscall(SYS_IOCTL, ...) are no longer allowed and
exported as of golang/sys@6fcdbc0bbc04

Also fix a build issue with GOOS=solaris and bump the
golang.org/x/sys/unix dependency.

/cc @thaJeztah

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, nice! Thank you! <3

@thaJeztah
Copy link
Member

@kolyshkin @cpuguy83 PTAL

Unify the implementation for all unix platforms to use IoctlGetTermios
and IoctlSetTermios from the golang.org/x/sys/unix package instead of
implementing it in terms of SYS_IOCTL. This avoids duplicating code,
makes error handling easier and fixes the build on darwin where direct
syscalls, i.e. unix.Syscall(SYS_IOCTL, ...) are no longer allowed and
exported as of golang/sys@6fcdbc0bbc04

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser
Copy link
Contributor Author

tklauser commented Sep 9, 2020

Thanks for the review @kolyshkin! Changed according to your comments, PTAL.

@jbowens
Copy link

jbowens commented Sep 11, 2020

@kolyshkin — is this good to merge? I'd love to see it merged.

Copy link

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Nice, LGTM.

@clementlecorre
Copy link

Thanks 👍

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

8 participants