Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

added a line that cleanup tap interface #1921

Merged
merged 1 commit into from Jan 5, 2016
Merged

added a line that cleanup tap interface #1921

merged 1 commit into from Jan 5, 2016

Conversation

il9ue
Copy link
Contributor

@il9ue il9ue commented Dec 27, 2015

Call RemovePersistentIface func to cleanup tap interface when error occurs.
Please give any feedback!

@jellonek
Copy link
Contributor

jellonek commented Jan 4, 2016

LGTM, but... Please also remove comment in line above added ;)

@il9ue il9ue force-pushed the master branch 2 times, most recently from 3051b07 to 608c522 Compare January 4, 2016 13:16
@il9ue
Copy link
Contributor Author

il9ue commented Jan 4, 2016

Thanks @jellonek, just removed the comment line above, and squashed.

@jellonek
Copy link
Contributor

jellonek commented Jan 5, 2016

@jonboulle IMO it's ready to be merged ;)

@@ -478,7 +478,7 @@ func kvmSetup(podRoot string, podID types.UUID, fps []ForwardedPort, netList com
}
err = netlink.LinkSetMaster(link, br)
if err != nil {
// TODO: cleanup tap interface
tuntap.RemovePersistentIface(n.runtime.IfName, tuntap.Tap)
Copy link
Member

Choose a reason for hiding this comment

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

Even if we ignore the error, should we print a warning with it if this fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. This is a good practice.

@il9ue
Copy link
Contributor Author

il9ue commented Jan 5, 2016

@iaguis @jellonek injected a error warning line to handle exception. Thanks.

@iaguis
Copy link
Member

iaguis commented Jan 5, 2016

LGTM. Thanks!

iaguis added a commit that referenced this pull request Jan 5, 2016
added a line that cleanup tap interface
@iaguis iaguis merged commit 5308a6d into rkt:master Jan 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants