signal: terminal: Changes to support build in Darwin #87

raravena80 · 2018-06-19T04:04:53Z

So, there's some code duplication between terminal_linux.go, terminal_linux_test.go and terminal_darwin.go and terminal_darwin_test.go. We can leave it as it is now, or we can refactor the code into perhaps one single terminal.go and then specific terminal_linux.go and then terminal_darwin.go. Refactoring will also require changing the test files.

Lmk

bergwolf

This is pretty close to terminal_linux.go, maybe we can have a common terminal.go that builds on both Linux and Darwin?

That said, the code is small enough to be kept both and we likely do not need to update it anyway. So I'm giving my LGTM.

jodh-intel

Hi @raravena80 - thanks for raising!

I like the idea of having a Darwin implementation.

A couple of things:

Please can you update .travis.yml so that we test on Linux and osx: https://docs.travis-ci.com/user/customizing-the-build#Mac-OS.
But also keep in mind: Remove TravisCI kata-containers#22.
If you haven't already seen it, you probably need to be aware of: Reduce footprint by implementing shim with other language #83.

jodh-intel · 2018-06-19T07:28:53Z

terminal_darwin.go

@@ -0,0 +1,52 @@
+// +build darwin
+//
+// Copyright (c) 2017 Intel Corporation


I see that this is based on terminal_linux.go but I think you need to add an additional 2018 copyright with your name against it. Is that correct @grahamwhaley?

I'll change it to 2017-2018 pending @grahamwhaley's comment.

correct - you may optionally want to add your own copyright line @raravena80 . But, don't update the Intel line to 2017-2018, if an Intel person didn't touch it ;-)

ok cool, I just left it like it was :)

jodh-intel · 2018-06-19T07:29:11Z

terminal_darwin.go

+}
+
+func restoreTerminal(fd int, termios *unix.Termios) error {
+	//return unix.IoctlSetTermios(fd, unix.TCSETS, termios)


Can you remove this commented-out code please.

sboeuf · 2018-06-19T17:04:34Z

signals.go

-	syscall.SIGILL:    true,
-	syscall.SIGQUIT:   true,
-	syscall.SIGSEGV:   true,
-	syscall.SIGSTKFLT: true,


@raravena80 could you please add a commit message explaining a bit what this patch is doing, and particularly the removal of SIGSTKFLT.
Maybe splitting this into two commits might help clarifying that too.

BTW, are you planning to implement what you mentioned in the issue? I mean having this signals.go as a common part between different OSes, and define signals_linux.go and signals_darwin.go for specific ones ?

@sboeuf looking for feedback. I'm fine the way it is, but looking to get the team's take on it.

Added the commit message with explanations for both signal and terminal. Let me know, I can also split these in two.

I think the simplest way would be to have signals_linux.go and signals_darwin.go with the build flag linux and darwin respectively. This way it is pretty clear what is being used depending on the OS, and I don't think the duplication is a problem here.

if you have the file name extentions (_$GOARCH), then you don't need the in-file tags, do you?

Oh interesting, looking here: https://golang.org/pkg/go/build/, you're right. But it cannot hurt to put the build flag on top of each file, just as an enforcement action.

Right, you don't need it but like @sboeuf mentioned it may be good to keep for informational purposes :)

Regarding build flag vs. filename, I'm not sure which one "trumps" the other. I'd rather we just use the filename as that's the convention we're using and we don't want to have to maintain two things for the same purpose ;)

sounds good. I just removed the +build header.

codecov · 2018-06-19T17:25:56Z

Codecov Report

Merging #87 into master will increase coverage by 2.46%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   50.96%   53.42%   +2.46%     
==========================================
  Files           6        7       +1     
  Lines         259      277      +18     
==========================================
+ Hits          132      148      +16     
- Misses        115      116       +1     
- Partials       12       13       +1

Impacted Files	Coverage Δ
signals.go	`64.7% <ø> (ø)`	⬆️
terminal_darwin.go	`88.88% <88.88%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 087a537...27339b0. Read the comment docs.

jodh-intel · 2018-06-20T16:05:02Z

lgtm

amshinde

lgtm

jodh-intel · 2018-06-21T08:50:27Z

Hi @raravena80 - Travis is failing and if you look at the log you'll see:

Found 2 commits between commit f14909849e2a66323ae617cb1af6651d12ac5520 and branch master
ERROR: Commit 095b68ab469922faa96a5a5f3cb637028e6345b6: subject too long (max 75, got 153): "signal: Removing SIGSTKFLT since it's not used in either the Linux or Darwin implementations. According to the man 7 page on Linux the signal is not used"
ERROR: checkcommits failed. See the document below for help on formatting
commits for the project.
https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md#patch-format

Hence, I suggest you change your commits to something like:

signal: Removed SIGSTKFLT

According to the man 7 page on Linux the signal is not used. Further, it is not defined on Darwin.

Signed-off-by: Ricardo Aravena <raravena@branch.io>

terminal: Add support for Darwin

Created a new `terminal_darwin.go` (and associated test).

Fixes #86

Signed-off-by: Ricardo Aravena <raravena@branch.io>

Also, please see my comment above about updating .travis.yml: #87 (review).

jodh-intel · 2018-06-21T08:51:32Z

In summary, we like the first commit line (the summary) to have a "subsystem" prefix and a very short description.

After a blank line, knock yourself out with full details! ;)

According to the man 7 page on Linux the signal is not used. Further, it is not defined on Darwin. Signed-off-by: Ricardo Aravena <raravena@branch.io>

raravena80 · 2018-06-21T16:40:49Z

@jodh-intel made the changes. Also added osx to travis.

Created a new `terminal_darwin.go` (and associated test). Made changes to .travis.yml and .ci/faketty.sh to support travis osx tests Fixes kata-containers#86 Signed-off-by: Ricardo Aravena <raravena@branch.io>

jodh-intel · 2018-06-22T09:34:13Z

Thanks @raravena80!

lgtm

bergwolf approved these changes Jun 19, 2018

View reviewed changes

jodh-intel reviewed Jun 19, 2018

View reviewed changes

sboeuf suggested changes Jun 19, 2018

View reviewed changes

raravena80 force-pushed the master branch from 72f69d2 to cb2eab1 Compare June 19, 2018 17:25

raravena80 force-pushed the master branch 3 times, most recently from c103180 to f149098 Compare June 20, 2018 15:58

amshinde reviewed Jun 20, 2018

View reviewed changes

amshinde approved these changes Jun 20, 2018

View reviewed changes

sboeuf approved these changes Jun 20, 2018

View reviewed changes

signal: Removed SIGSTKFLT

937a9a3

According to the man 7 page on Linux the signal is not used. Further, it is not defined on Darwin. Signed-off-by: Ricardo Aravena <raravena@branch.io>

raravena80 force-pushed the master branch from f149098 to 6103bb1 Compare June 21, 2018 16:40

raravena80 force-pushed the master branch 3 times, most recently from 6667122 to 27339b0 Compare June 21, 2018 18:30

terminal: Add support for Darwin

27339b0

Created a new `terminal_darwin.go` (and associated test). Made changes to .travis.yml and .ci/faketty.sh to support travis osx tests Fixes kata-containers#86 Signed-off-by: Ricardo Aravena <raravena@branch.io>

jodh-intel merged commit edc8e40 into kata-containers:master Jun 22, 2018

jodh-intel mentioned this pull request Jun 26, 2018

Zuul CI tracking issue kata-containers/ci#42

Closed

jodh-intel mentioned this pull request Sep 4, 2018

travis: add linux-ppc64le and osx kata-containers/tests#692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signal: terminal: Changes to support build in Darwin #87

signal: terminal: Changes to support build in Darwin #87

raravena80 commented Jun 19, 2018 •

edited

Loading

bergwolf left a comment

jodh-intel left a comment

jodh-intel Jun 19, 2018

raravena80 Jun 19, 2018

grahamwhaley Jun 19, 2018

raravena80 Jun 19, 2018

jodh-intel Jun 19, 2018

sboeuf Jun 19, 2018

sboeuf Jun 19, 2018

raravena80 Jun 19, 2018

raravena80 Jun 19, 2018

sboeuf Jun 19, 2018

grahamwhaley Jun 19, 2018

sboeuf Jun 19, 2018

raravena80 Jun 19, 2018

jodh-intel Jun 20, 2018

raravena80 Jun 20, 2018

codecov bot commented Jun 19, 2018 •

edited

Loading

jodh-intel commented Jun 20, 2018 •

edited by amshinde

Loading

amshinde left a comment

jodh-intel commented Jun 21, 2018

jodh-intel commented Jun 21, 2018

raravena80 commented Jun 21, 2018

jodh-intel commented Jun 22, 2018

signal: terminal: Changes to support build in Darwin #87

signal: terminal: Changes to support build in Darwin #87

Conversation

raravena80 commented Jun 19, 2018 • edited Loading

bergwolf left a comment

Choose a reason for hiding this comment

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2018 • edited Loading

Codecov Report

jodh-intel commented Jun 20, 2018 • edited by amshinde Loading

amshinde left a comment

Choose a reason for hiding this comment

jodh-intel commented Jun 21, 2018

jodh-intel commented Jun 21, 2018

raravena80 commented Jun 21, 2018

jodh-intel commented Jun 22, 2018

raravena80 commented Jun 19, 2018 •

edited

Loading

codecov bot commented Jun 19, 2018 •

edited

Loading

jodh-intel commented Jun 20, 2018 •

edited by amshinde

Loading