Skip to content

Cleanup iptables after bridge network is removed#591

Merged
aboch merged 1 commit intomoby:masterfrom
WeiZhang555:iptables-clean
Nov 25, 2015
Merged

Cleanup iptables after bridge network is removed#591
aboch merged 1 commit intomoby:masterfrom
WeiZhang555:iptables-clean

Conversation

@WeiZhang555
Copy link
Copy Markdown
Contributor

Fixed #570

Clean unused iptables rules after bridge network is removed

Signed-off-by: Zhang Wei zhangwei555@huawei.com

@WeiZhang555
Copy link
Copy Markdown
Contributor Author

Sorry for the late, I'm still on vacation and have only limited Internet visit, and this situation will suppose to last another 3 days.
Tell me what do you think of this changes, I'll handle it in time if possible. @mavenugo

@WeiZhang555 WeiZhang555 force-pushed the iptables-clean branch 2 times, most recently from dee34e0 to 63b147d Compare October 10, 2015 06:57
@WeiZhang555
Copy link
Copy Markdown
Contributor Author

Just rebased, let me know what do you think of this change :)
ping @mavenugo

@mavenugo
Copy link
Copy Markdown
Contributor

Thanks @WeiZhang555 . Will take a look at it shortly.

@WeiZhang555
Copy link
Copy Markdown
Contributor Author

It has been a while since last update, how's this going? Any suggestion on this? 😄

ping @mavenugo

Comment thread drivers/bridge/setup_ip_tables.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You don't need to define a defer function here and no err == nil check. You only need to register the cleanup function unconditionally, because there is no further code which sets the err variable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unless the execution order of the cleanup function is important. If that is the case, then the last defer is needed as it will assure we cleanup in reverse order what we configured.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. I'll remove the defer, and defer on line 71 seems unnecessary too, I'll remove it together.

@WeiZhang555
Copy link
Copy Markdown
Contributor Author

Thanks for your detailed and patient suggestion @aboch , I have made some changes per your suggestion, please take a look. 😄

Fixed moby#570

Clean unused iptables rules after bridge network is removed

Signed-off-by: Zhang Wei <zhangwei555@huawei.com>
@WeiZhang555
Copy link
Copy Markdown
Contributor Author

It seems a long time has passed, how's this going?
Ping @mavenugo @aboch @mrjana

@mavenugo
Copy link
Copy Markdown
Contributor

@aboch can you please wrap up your review on this one ?

@mavenugo
Copy link
Copy Markdown
Contributor

LGTM.
ping @aboch

@aboch
Copy link
Copy Markdown
Contributor

aboch commented Nov 25, 2015

Thanks @WeiZhang555 for the fix and taking care of comments.
And sorry for the delay on this.

LGTM

aboch added a commit that referenced this pull request Nov 25, 2015
Cleanup iptables after bridge network is removed
@aboch aboch merged commit a06a141 into moby:master Nov 25, 2015
@WeiZhang555
Copy link
Copy Markdown
Contributor Author

I'm always happy to help 😄

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants