KPty: add posix_openpt support and imporve openpty support #108

Merged
SlavekB merged 2 commits from feat/fix-openpty-conditional into master 4 years ago
obache commented 4 years ago
Collaborator

Add support to open master pseudo terminal device with posix standard posix_openpt.

Fix KPty::open preventing to open slave pseudo ttys twice for openpty platforms.

Add support to open master pseudo terminal device with posix standard `posix_openpt`. Fix `KPty::open` preventing to open slave pseudo ttys twice for `openpty` platforms.
MicheleC approved these changes 4 years ago
MicheleC left a comment
Owner

Looks good.

Looks good.
Owner

@SlavekB do you want to test build in BSD before merging?

@SlavekB do you want to test build in BSD before merging?
MicheleC requested review from SlavekB 4 years ago
SlavekB reviewed 4 years ago
SlavekB left a comment
Owner

It looks good, but I have one question there – see the comment.

It looks good, but I have one question there – see the comment.
_tcsetattr(d->slaveFd, &ttmode);
#ifndef HAVE_OPENPTY
Owner

The previous ioctl call to set screen size is conditioned by #ifdef HAVE_OPENPTY, while this call is exactly the opposite – #ifndef HAVE_OPENPTY. It is not clear to me why this is the case. Please, can you explain that to me?

The previous ioctl call to set screen size is conditioned by `#ifdef HAVE_OPENPTY`, while this call is exactly the opposite – `#ifndef HAVE_OPENPTY`. It is not clear to me why this is the case. Please, can you explain that to me?
obache commented 4 years ago
Poster
Collaborator

Screen size should be already set with openpty call in KPty::open, but for KPty::setPty case (d->slaveFd == -1), it is unsure how masterFd is opened, i.e. how KPty::setPty is used.

Screen size should be already set with `openpty` call in `KPty::open`, but for `KPty::setPty` case (d->slaveFd == -1), it is unsure how `masterFd` is opened, i.e. how `KPty::setPty` is used.
Owner

@SlavekB do you want to test build in BSD before merging?

Tested on FreeBSD – build is successful.

> @SlavekB do you want to test build in BSD before merging? Tested on FreeBSD – build is successful.
SlavekB approved these changes 4 years ago
SlavekB left a comment
Owner

Ok, it looks ready to merge.

Ok, it looks ready to merge.
SlavekB merged commit 8f7371cddf into master 4 years ago
SlavekB deleted branch feat/fix-openpty-conditional 4 years ago
SlavekB added this to the R14.0.9 release milestone 4 years ago

Reviewers

MicheleC approved these changes 4 years ago
SlavekB approved these changes 4 years ago
The pull request has been merged as 8f7371cddf.
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/tdelibs#108
Loading…
There is no content yet.