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

fix panic on get container pid when live restore containers #35157

Merged
merged 1 commit into from Oct 17, 2017

Conversation

Projects
None yet
5 participants
@BSWANG
Contributor

BSWANG commented Oct 10, 2017

Signed-off-by: bingshen.wbs bingshen.wbs@alibaba-inc.com

- What I did
fix panic on get container pid when live restore containers

- How I did it
Avoid use nil container instance.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

fix panic on get container pid when live restore containers
Signed-off-by: bingshen.wbs <bingshen.wbs@alibaba-inc.com>
@thaJeztah

LGTM

@@ -84,7 +84,7 @@ func (daemon *Daemon) getPidContainer(container *container.Container) (*containe
containerID := container.HostConfig.PidMode.Container()
container, err := daemon.GetContainer(containerID)

This comment has been minimized.

@thaJeztah

thaJeztah Oct 11, 2017

Member

Wondering if we should change this variable's name back to just c, as I think it now masks the container argument (this was also changed in 12485d6)

There's two containers in this function: the "main" container, and the container which namespace it's joining.

(I see that commit changed the variable name in a couple of other functions as well)

@thaJeztah

thaJeztah Oct 11, 2017

Member

Wondering if we should change this variable's name back to just c, as I think it now masks the container argument (this was also changed in 12485d6)

There's two containers in this function: the "main" container, and the container which namespace it's joining.

(I see that commit changed the variable name in a couple of other functions as well)

This comment has been minimized.

@BSWANG

BSWANG Oct 12, 2017

Contributor

Yep, We should change this variable's name to just c. But I think we should open an new PR to do this. : )

@BSWANG

BSWANG Oct 12, 2017

Contributor

Yep, We should change this variable's name to just c. But I think we should open an new PR to do this. : )

This comment has been minimized.

@thaJeztah

thaJeztah Oct 12, 2017

Member

Agreed!

@thaJeztah

thaJeztah Oct 12, 2017

Member

Agreed!

@boaz1337

LGTM 👍

@cpuguy83

LGTM, but am not sure that is the correct error message overall.... can change separately.

@cpuguy83 cpuguy83 merged commit fa2df86 into moby:master Oct 17, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37330 has succeeded
Details
janky Jenkins build Docker-PRs 46013 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6396 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17580 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6202 has succeeded
Details

@BSWANG BSWANG deleted the BSWANG:fix-panic-on-restore branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment