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

Use named error return for initProcess#start #2238

Merged
merged 1 commit into from
Mar 10, 2020

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Feb 27, 2020

In initProcess#start, there is deferred func :

	defer func() {
		if err != nil {

Since the error return is not named, the above check doesn't cover all the cases.
e.g. a few lines below:

	if _, err := io.Copy(p.messageSockPair.parent, p.bootstrapData); err != nil {
		return newSystemErrorWithCause(err, "copying bootstrap data to pipe")

The check in deferred func doesn't check either err or, the actual returned error.

This PR changes the error return to be named.

@tedyu tedyu force-pushed the init-proc-err-ret branch from 7b64d10 to a230cbd Compare February 27, 2020 20:20
@tedyu
Copy link
Contributor Author

tedyu commented Feb 27, 2020

@hqhq @cyphar
Mind taking a look ?

@tedyu
Copy link
Contributor Author

tedyu commented Feb 27, 2020

/cc @mrunalp

@tedyu
Copy link
Contributor Author

tedyu commented Feb 28, 2020

The deferred func starting line 352 is duplicate with the one starting line 304.

I think the second deferred func can be removed.

@tedyu
Copy link
Contributor Author

tedyu commented Feb 28, 2020

What happens is that the second deferred func is ineffective.
I checked every err variable after line 352: they are all local (newly declared) to the operation.

I will drop the second deferred func.

@kolyshkin
Copy link
Contributor

@tedyu thanks for the patch! Coincidentally, I was looking into this part of code as well and ended up with a similar patch.

Some discussion about the code is provided at #1916 (review) (also, this is a PR to blame) and yet it doesn't explain the double defer.

I think the defer needs to be moved up a bit (before p.intelRdtManager.Apply() or even before p.manager.Apply()), otherwise, if p.intelRdtManager.Apply() fails, p.manager.Destroy() is not called.

@crosbymichael @giuseppe PTAL

@mrunalp
Copy link
Contributor

mrunalp commented Mar 4, 2020

LGTM

Approved with PullApprove

@mrunalp
Copy link
Contributor

mrunalp commented Mar 4, 2020

@AkihiroSuda ptal

@tedyu tedyu force-pushed the init-proc-err-ret branch from b56cc49 to a37c17a Compare March 4, 2020 08:36
@tedyu
Copy link
Contributor Author

tedyu commented Mar 4, 2020

@kolyshkin
I have lifted the deferred func earlier.
Please take a look.

@tedyu
Copy link
Contributor Author

tedyu commented Mar 5, 2020

@kolyshkin
Please let me know if there is anything else to be addressed.

@thaJeztah
Copy link
Member

@crosbymichael ptal 🤗

@crosbymichael
Copy link
Member

crosbymichael commented Mar 6, 2020

LGTM

Approved with PullApprove

@tedyu
Copy link
Contributor Author

tedyu commented Mar 9, 2020

@crosbymichael @thaJeztah
Can this be merged ?

Thanks

@thaJeztah
Copy link
Member

I'm not a maintainer in this repository; @AkihiroSuda @mrunalp PTAL?

@tedyu tedyu force-pushed the init-proc-err-ret branch from a37c17a to 957da1f Compare March 9, 2020 16:29
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 9, 2020

LGTM

Approved with PullApprove

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

/lgtm

@tedyu
Copy link
Contributor Author

tedyu commented Mar 10, 2020

@mrunalp
Can you merge this ?

thanks

@AkihiroSuda AkihiroSuda merged commit 71dfb55 into opencontainers:master Mar 10, 2020
@tedyu
Copy link
Contributor Author

tedyu commented Mar 10, 2020

Thanks @AkihiroSuda

@AkihiroSuda
Copy link
Member

Signed-off-by: zyu

Please use your full name next time

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

Successfully merging this pull request may close these issues.

6 participants