2627 fix "some of" golangci-lint problems #2641

ehsundar · 2020-10-10T21:55:07Z

this request is related to this issue

kolyshkin · 2020-10-11T21:57:44Z

contrib/cmd/recvtty/recvtty.go

 	os.Exit(1)
 }

-func handleSingle(path string, noStdin bool) error {
+func handleSingle(path string, noStdin bool) (err error) {


I'd suggest calling it retErr or so, to distinguish between errors.

Err is what I usually use.

kolyshkin · 2020-10-11T21:58:42Z

contrib/cmd/recvtty/recvtty.go

 	// Open a socket.
 	ln, err := net.Listen("unix", path)
 	if err != nil {
 		return err
 	}
-	defer ln.Close()
+	defer func() {
+		err = ln.Close()


So, what would happen if net.Listen fails? Please be careful.

I was wrong here (except that you rewriting the error).

No need to metion the defer part will run after func returns. so if any error happens in closing, the retErr will be refilled. the question is: is it important to you to handle this kind of errors in the caller function?

There is also another problem: if the func returns because of another error, and at the same time, the close also fails, the former error will be lost! while the closing error is bound (in my opinion) I believe the original error is more significant ... tell me what to do?

I decided to ignore closing error...

kolyshkin · 2020-10-11T21:58:51Z

contrib/cmd/recvtty/recvtty.go


 	// We only accept a single connection, since we can only really have
 	// one reader for os.Stdin. Plus this is all a PoC.
 	conn, err := ln.Accept()
 	if err != nil {
 		return err
 	}
-	defer conn.Close()
+	defer func() {
+		err = conn.Close()


There are several changes of this form, and while Close can fail very few Go programs handle this case successfully all the time and you would need to write the same boilerplate for each case:

defer func() { if Err != nil { Err = foo.Close() } }()

Aside from adding a bunch of boilerplate to lots of functions, the second question is when will close(2) fail and does it make sense to deal with this? Most close failures are actually when the filesystem had an error writing to disk, but this mechanism in Linux is unpredictable and ultimately there's not much to do if that's why close(2) failed (very often the failure happens during a later fsync(2)). Most other close(2) failures don't apply to most Go programs. Not to mention that the worst thing that happens is that we keep a file descriptor open in a program which takes about 100ms to run -- and we can't do anything about it anyway because the semantics of close(2) are that you cannot safely attempt to re-close a file after calling close(2).

We can do it, but I don't see the benefit and if the only reason for this change is to make a CI lint happy, we should turn off the CI lint IMHO.

Yeah, the alternative that is not too much fuss and makes static checkers happy is same as in (void)write(); in C, i.e.

defer func() { _ := foo.Close() }

Thanks for your expressive description. As a boilerplate, I may suggest this one:

defer func() { if err = foo.Close(); err != nil && retErr != nil { retErr = err } }()

because we should try to close() anyways. but its much more than its needed (I think), and as @kolyshkin said, I'll try to simply ignore the closing error across my MR

kolyshkin · 2020-10-11T22:00:27Z

contrib/cmd/recvtty/recvtty.go

@@ -61,28 +61,35 @@ terminals:
 )

 func bail(err error) {
-	fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err)
+	_, _ = fmt.Fprintf(os.Stderr, "[recvtty] fatal error: %v\n", err)


This change is not inline with what the commit message says. Please split your commits carefully.

libcontainer/configs/config_test.go

kolyshkin · 2020-10-11T22:03:23Z

Thank you for PR. I left a few suggestions.

In general, I would like it to be split into smaller focused patches (I see you already did that), and make sure your code is correct.

ehsundar · 2020-10-15T17:57:56Z

Sorry for being late, I didn't receive any email or notif. I should have configured github before. thanks

Signed-off-by: Amir Mohamad Ehsandar <[email protected]>

ehsundar · 2020-10-15T18:36:50Z

I tried to apply your suggestions. and I tried to split my commits a little more. sorry for making mistakes. It's the first time I contribute to an open source repo. I really didn't expect this precise review. thank you.

kolyshkin · 2020-10-20T19:06:22Z

contrib/cmd/recvtty/recvtty.go

 	}
 	if err := console.ClearONLCR(c.Fd()); err != nil {
 		return err
 	}

 	// Copy from our stdio to the master fd.
 	quitChan := make(chan struct{})
+	errChan := make(chan error)


OK it seems that in this case a single channel is enough, there is no need to have two.

kolyshkin · 2020-10-20T19:08:12Z

contrib/cmd/recvtty/recvtty.go

-		return err
+	ln, retErr := net.Listen("unix", path)
+	if retErr != nil {
+		return retErr


You did't have to change any of this.

IOW this can stay err -- the only place to use retErr is the defer.

kolyshkin · 2020-10-20T19:09:08Z

contrib/cmd/recvtty/recvtty.go

-		return err
+	conn, retErr := ln.Accept()
+	if retErr != nil {
+		return


kolyshkin · 2020-10-20T19:10:07Z

contrib/cmd/recvtty/recvtty.go

-		return err
+	socket, retErr := unixconn.File()
+	if retErr != nil {
+		return


kolyshkin · 2020-10-20T19:10:19Z

contrib/cmd/recvtty/recvtty.go

-		return err
+	c, retErr := console.ConsoleFromFile(master)
+	if retErr != nil {
+		return


kolyshkin · 2020-10-20T19:22:59Z

contrib/cmd/recvtty/recvtty.go

+		_, err := io.Copy(os.Stdout, c)
+		if err != nil {
+			errChan <- err
+		}
 		quitChan <- struct{}{}


IOW this could be simple

_, err := io.Copy(os.Stdout, c) chan <- err

and the code that's receiving this will check for nil

kolyshkin · 2020-11-04T16:11:06Z

@ehsundar please let us know if you're going to keep working on that or not

AkihiroSuda · 2020-11-09T06:33:35Z

@kolyshkin Would you be interested in carrying this PR? 🙏

kolyshkin · 2020-11-17T00:45:52Z

@kolyshkin Would you be interested in carrying this PR?

Looks like I have no choice :) so yes, I will :)

AkihiroSuda · 2020-11-17T06:01:32Z

Thanks!

kolyshkin · 2020-11-30T19:06:05Z

kolyshkin · 2020-11-30T19:08:43Z

Not using any patches from this one since it's easier to fix from scratch, thus closing. The issue #2627 is still open though.

kolyshkin · 2020-12-03T16:34:15Z

superseded by the above PRs

ehsundar force-pushed the 2627-fix-cilint-probs branch from 0a028c3 to 1a1b7c8 Compare October 10, 2020 21:56

ehsundar changed the title ~~2627 fix cilint probs~~ 2627 fix "some of" golangci-lint problems Oct 11, 2020

kolyshkin reviewed Oct 11, 2020

View reviewed changes

libcontainer/configs/config_test.go Outdated Show resolved Hide resolved

ehsundar force-pushed the 2627-fix-cilint-probs branch from 1a1b7c8 to 81a080f Compare October 15, 2020 17:23

ehsundar force-pushed the 2627-fix-cilint-probs branch 2 times, most recently from c4d3374 to 56b1aa8 Compare October 15, 2020 18:33

ehsundar requested review from cyphar and kolyshkin October 15, 2020 18:33

ehsundar added 4 commits October 15, 2020 22:03

Use error channel to catch errors in unsupervised goroutines

c3b31e7

Signed-off-by: Amir Mohamad Ehsandar <[email protected]>

Handle unhandled errors to prevent linter warnings in recvtty.go

a3bd52b

Signed-off-by: Amir Mohamad Ehsandar <[email protected]>

Fix config.go linter errors by ignoring them

2735709

Signed-off-by: Amir Mohamad Ehsandar <[email protected]>

Fix simplification linter errors

56b1aa8

Signed-off-by: Amir Mohamad Ehsandar <[email protected]>

kolyshkin reviewed Oct 20, 2020

View reviewed changes

contrib/cmd/recvtty/recvtty.go

return err

conn, retErr := ln.Accept()

if retErr != nil {

return

Copy link

Contributor

kolyshkin Oct 20, 2020

Choose a reason for hiding this comment

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

same here

kolyshkin reviewed Oct 20, 2020

View reviewed changes

contrib/cmd/recvtty/recvtty.go

return err

socket, retErr := unixconn.File()

if retErr != nil {

return

Copy link

Contributor

kolyshkin Oct 20, 2020

Choose a reason for hiding this comment

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

same here

kolyshkin reviewed Oct 20, 2020

View reviewed changes

contrib/cmd/recvtty/recvtty.go

return err

c, retErr := console.ConsoleFromFile(master)

if retErr != nil {

return

Copy link

Contributor

kolyshkin Oct 20, 2020

Choose a reason for hiding this comment

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

same here

kolyshkin reviewed Oct 20, 2020

View reviewed changes

kolyshkin closed this Dec 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2627 fix "some of" golangci-lint problems #2641

2627 fix "some of" golangci-lint problems #2641

ehsundar commented Oct 10, 2020 •

edited

Loading

kolyshkin Oct 11, 2020

cyphar Oct 12, 2020

kolyshkin Oct 11, 2020

kolyshkin Oct 13, 2020

ehsundar Oct 15, 2020

ehsundar Oct 15, 2020

ehsundar Oct 15, 2020

kolyshkin Oct 11, 2020

cyphar Oct 12, 2020

kolyshkin Oct 13, 2020

ehsundar Oct 15, 2020

kolyshkin Oct 11, 2020

kolyshkin commented Oct 11, 2020

ehsundar commented Oct 15, 2020

ehsundar commented Oct 15, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin Oct 20, 2020

kolyshkin commented Nov 4, 2020

AkihiroSuda commented Nov 9, 2020

kolyshkin commented Nov 17, 2020

AkihiroSuda commented Nov 17, 2020

kolyshkin commented Nov 30, 2020 •

edited

Loading

kolyshkin commented Nov 30, 2020

kolyshkin commented Dec 3, 2020

2627 fix "some of" golangci-lint problems #2641

2627 fix "some of" golangci-lint problems #2641

Conversation

ehsundar commented Oct 10, 2020 • edited Loading

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

kolyshkin commented Oct 11, 2020

ehsundar commented Oct 15, 2020

ehsundar commented Oct 15, 2020

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

kolyshkin commented Nov 4, 2020

AkihiroSuda commented Nov 9, 2020

kolyshkin commented Nov 17, 2020

AkihiroSuda commented Nov 17, 2020

kolyshkin commented Nov 30, 2020 • edited Loading

kolyshkin commented Nov 30, 2020

kolyshkin commented Dec 3, 2020

ehsundar commented Oct 10, 2020 •

edited

Loading

kolyshkin commented Nov 30, 2020 •

edited

Loading