Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MEN-3974 #33

Merged
merged 2 commits into from
Sep 30, 2020
Merged

MEN-3974 #33

merged 2 commits into from
Sep 30, 2020

Conversation

oleorhagen
Copy link

No description provided.

Changelog: None
Signed-off-by: Ole Petter <ole.orhagen@northern.tech>
Copy link

@lluiscampos lluiscampos left a comment

Choose a reason for hiding this comment

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

My concern is that this segfault can happen with production code, not only with this test. The test is anyway skipped in the Debian build with a custom patch, so this PR is yet another workaround then.

Have we figure out something in the underlying code? When I debugged it it was very strange: conn was actually nil, and that should not be possible!

@oleorhagen
Copy link
Author

oleorhagen commented Sep 30, 2020

My concern is that this segfault can happen with production code, not only with this test. The test is anyway skipped in the Debian build with a custom patch, so this PR is yet another workaround then.

I see. The second commit can probably be skipped then.

Have we figure out something in the underlying code? When I debugged it it was very strange: conn was actually nil, and that should not be possible!

I was worried at first too, but:

It was because the error was not checked. If err != nil => conn = nil. So as long as our code does it's error checking properly, this is not an issue.

I added this too the tests also, as an extra: b0c8095

Changelog: None
Signed-off-by: Ole Petter <ole.orhagen@northern.tech>
@oleorhagen
Copy link
Author

Update was simply gofmt.

@lluiscampos
Copy link

I was worried at first too, but:

It was because the error was not checked. If err != nil => conn = nil. So as long as our code does it's error checking properly, this is not an issue.

That makes sense, thanks.

@oleorhagen
Copy link
Author

@lluiscampos regarding the second commit. Leave it, or remove?

@lluiscampos
Copy link

@oleorhagen Leave it, it is harmless. I'll remove the other patch from Debian in the next update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants