Skip to content

Use named error return for initProcess#start#2238

Merged
AkihiroSuda merged 1 commit intoopencontainers:masterfrom
tedyu:init-proc-err-ret
Mar 10, 2020
Merged

Use named error return for initProcess#start#2238
AkihiroSuda merged 1 commit intoopencontainers:masterfrom
tedyu:init-proc-err-ret

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.

@tedyu tedyu force-pushed the init-proc-err-ret branch from a230cbd to b56cc49 Compare February 28, 2020 00:59
@thaJeztah thaJeztah mentioned this pull request Mar 3, 2020
Merged
@kolyshkin
Copy link
Contributor

kolyshkin commented Mar 4, 2020

@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 •
edited by AkihiroSuda
Loading

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

thaJeztah commented Mar 6, 2020

@crosbymichael ptal 🤗

@crosbymichael
Copy link
Member

crosbymichael commented Mar 6, 2020 •
edited by AkihiroSuda
Loading

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

thaJeztah commented Mar 9, 2020

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

AkihiroSuda
AkihiroSuda reviewed Mar 9, 2020
View reviewed changes
@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 •
edited
Loading

LGTM

Approved with PullApprove

kolyshkin
kolyshkin approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment •
edited
Loading

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

AkihiroSuda commented Mar 10, 2020

Signed-off-by: zyu

Please use your full name next time

@kolyshkin kolyshkin mentioned this pull request Jun 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@kolyshkin kolyshkin kolyshkin approved these changes

Assignees

No one assigned

Labels

kind/bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tedyu @kolyshkin @mrunalp @thaJeztah @crosbymichael @AkihiroSuda