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

Wait for criuProcess once #2420

Merged
merged 1 commit into from
May 29, 2020
Merged

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented May 19, 2020

In linuxContainer#criuSwrk(), the deferred func may wait on criuProcess for a second time, if the following has been executed (around line 1640):

	st, err = criuProcess.Wait()

This PR adds check for st so that there is no redundant wait.

Note: there is already deferred criuClientCon.Close() on line 1476

@kolyshkin
Copy link
Contributor

  1. I do not understand the meaning of
                if err != nil {
                        return
                }

in the defer func(). Looks like it does nothing, so it does not make sense to have it at all (unless I am mistaken and it actually means something).

  1. Since we are ignoring the error, there is nothing wrong to call Wait for a second time.

  2. I would remove the second "criuClientCon.Close()" (as a separate patch, first in the series).

@kolyshkin
Copy link
Contributor

@avagin PTAL

@tedyu tedyu force-pushed the criu-proc-wait branch from 050fd19 to 486065e Compare May 19, 2020 22:56
@tedyu
Copy link
Contributor Author

tedyu commented May 19, 2020

w.r.t. #1, I have updated the PR to log the error.

@tedyu
Copy link
Contributor Author

tedyu commented May 21, 2020

Ping @avagin

@avagin
Copy link
Contributor

avagin commented May 29, 2020

LGTM

@tedyu tedyu force-pushed the criu-proc-wait branch from a9fa42b to 0b589ab Compare May 29, 2020 17:58
@kolyshkin
Copy link
Contributor

kolyshkin commented May 29, 2020

LGTM

Approved with PullApprove

@kolyshkin
Copy link
Contributor

@AkihiroSuda @mrunalp PTAL

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

Signed-off-by: Ted Yu <yuzhihong@gmail.com>
@tedyu tedyu force-pushed the criu-proc-wait branch from 0b589ab to 3ba3d9b Compare May 29, 2020 22:50
Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin merged commit 774a9e7 into opencontainers:master May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants