Skip to content

Forbid adding duplicated namespace #1169

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.

Sign up for GitHub

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

Closed
yummypeng wants to merge 1 commit into from
Closed

Forbid adding duplicated namespace #1169

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions libcontainer/configs/namespaces_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ func (n *Namespaces) Remove(t NamespaceType) bool {
return true
}

func (n *Namespaces) Add(t NamespaceType, path string) {
func (n *Namespaces) Add(t NamespaceType, path string) bool {
i := n.index(t)
if i == -1 {
*n = append(*n, Namespace{Type: t, Path: path})
return
return true
}
(*n)[i].Path = path
return false
}
Copy link
Member

@cyphar cyphar Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

You haven't changed any of the callers of .Add to check the return value, so we're now ignoring duplicates. Please fix that.

Copy link
Contributor

@WeiZhang555 WeiZhang555 Nov 1, 2016

Choose a reason for hiding this comment

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

I don't think this is right.

This function is not only for adding new namespaces, but also for modifying path of existing namespace, you shouldn't disable this function. Also as @cyphar said, you didn't modify any caller so the return false/true is meaningless.

Copy link
Author

@yummypeng yummypeng Nov 1, 2016

Choose a reason for hiding this comment

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

Except the code I quoted before, I only find the callers of Add() in some test code. I know that this function is also used for modifying path of existing namespace, but I still think it would be better that if we want to modify path of existing namespace, we should remove it first, and then add it. If this idea is acceptable, I will fix those test functions then.

Copy link
Author

@yummypeng yummypeng Nov 1, 2016

Choose a reason for hiding this comment

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

Hmmmm.....If we must remove and then add to modify path of an existing namespace, then I think we won't need any check in Add().

Copy link
Member

@cyphar cyphar Nov 1, 2016

Choose a reason for hiding this comment

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

Actually, I'm confused. The code you quoted already doesn't allow for duplicate namespaces -- what's the benefit of this PR (it also appears to have broken the tests)?

Copy link
Author

@yummypeng yummypeng Nov 1, 2016

Choose a reason for hiding this comment

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

At the beginning, I just mistakenly thought the last line of code in Add() is redundant. But now it seems that my PR is meaningless and it's better to remain unchanged. Thanks for all your comments. I will close this PR later.


func (n *Namespaces) index(t NamespaceType) int {
Expand Down