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.

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

Jump to bottom

validate: Code optimization #486

Closed
wants to merge 1 commit into from
Closed

validate: Code optimization #486

wants to merge 1 commit into from

Conversation

zhouhao3
Copy link

Make the error message more specific.

Signed-off-by: zhouhao zhouhao@cn.fujitsu.com

Mashimiao
Mashimiao reviewed Sep 28, 2017
View reviewed changes
validate/validate.go Outdated
}

if ns.Path != "" && !filepath.IsAbs(ns.Path) {
return false
return fmt.Errorf("%v is not an absolute path", ns.Path)
Copy link

Choose a reason for hiding this comment

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

maybe fmt.Errorf(path %v of namespace %v is not absolute, ns.Type, ns.Path) better?

Copy link
Author

Choose a reason for hiding this comment

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

Fine.

wking
wking reviewed Sep 28, 2017
View reviewed changes
validate/validate.go Outdated
@@ -903,14 +903,14 @@ func namespaceValid(ns rspec.LinuxNamespace) bool {
case rspec.UserNamespace:
case rspec.CgroupNamespace:
default:
return false
return fmt.Errorf("invalid namespace type %s", ns.Type)
Copy link
Contributor

@wking wking Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The spec actually has no RFC 2119 wording for this check, saying only “The following namespace types are supported”. It doesn't say that runtimes MUST support them, or that they MUST NOT support additional values, or even that configurations SHOULD use values from that list. I'd guess the spec-maintainer intention was closer to:

Configurations SHOULD use, and runtimes SHOULD support, the following values:

Compliance testing would be easier with “runtimes MUST support”, but see opencontainers/runtime-spec#813.

Anyhow, from the config-validation side, I'd be happier seeing this supported by an effective-SHOULD, similar to what I've proposed in #481.

wking
wking reviewed Sep 28, 2017
View reviewed changes
validate/validate.go Outdated
}

if ns.Path != "" && !filepath.IsAbs(ns.Path) {
return false
return fmt.Errorf("path %v of namespace %v is not absolute path", ns.Path, ns)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the 1.0 spec is clearer on this point, but I've filed opencontainers/runtime-spec#925 to attach a MUST to this condition. I'm fine treating the requirement as an implicit-MUST in the meantime.

@wking
Copy link
Contributor

wking commented Sep 28, 2017

This PR belongs in v0.3.0. It's not using specerror.NewError yet, but returning an error from namespaceValid is necessary groundwork for future NewError use.

@zhouhao3 zhouhao3 force-pushed the namespec-fix branch 2 times, most recently from 2711261 to 674d195 Compare October 18, 2017 06:45
@zhouhao3
Copy link
Author

updated, @Mashimiao @liangchenye @wking PTAL

liangchenye
liangchenye reviewed Oct 18, 2017
View reviewed changes
specerror/config-linux.go Outdated
)

func init() {
register(DefaultFilesystems, rfc2119.Should, defaultFilesystemsRef)
register(NamespacesType, rfc2119.Should, namespacesRef)
Copy link
Member

Choose a reason for hiding this comment

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

The spec says: "The following namespace types are supported", We don't have key words like 'SHOULD'. Maybe we can add a PR to spec project?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we can add a PR to spec project?

Approve.

Copy link
Author

Choose a reason for hiding this comment

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

I open opencontainers/runtime-spec#931.

liangchenye
liangchenye reviewed Oct 18, 2017
View reviewed changes
specerror/config-linux.go Outdated
@@ -10,14 +10,21 @@ import (
const (
// DefaultFilesystems represents "The following filesystems SHOULD be made available in each container's filesystem:"
DefaultFilesystems = "The following filesystems SHOULD be made available in each container's filesystem:"
NamespacesType = "The following namespace types are supported:"
NamespacesPath = "This value MUST be an absolute path."
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to extend it to "This value MUST be an absolute path in the runtime mount namespace. "
There are several 'an absolute path' RFCs, https://github.com/opencontainers/runtime-tools/blob/master/specerror/config.go#L32, https://github.com/opencontainers/runtime-tools/blob/master/specerror/config.go#L42.
It will be easier to differentiate them.

liangchenye
liangchenye reviewed Oct 18, 2017
View reviewed changes
specerror/config-linux.go Outdated
@@ -10,14 +10,21 @@ import (
const (
// DefaultFilesystems represents "The following filesystems SHOULD be made available in each container's filesystem:"
DefaultFilesystems = "The following filesystems SHOULD be made available in each container's filesystem:"
NamespacesType = "The following namespace types are supported:"
NamespacesPath = "This value MUST be an absolute path."
Copy link
Member

@liangchenye liangchenye Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

rename to NamespacePathAbs?

The naming is a problem, maybe we can add a document to specerror directory to tell our naming rule.

@zhouhao3 zhouhao3 mentioned this pull request Oct 18, 2017
Closed
@zhouhao3 zhouhao3 force-pushed the namespec-fix branch 4 times, most recently from 0fc41b9 to 57601a4 Compare November 1, 2017 02:26
@liangchenye
Copy link
Member

rebase required

validate: Code optimization
a6345a3 
Signed-off-by: zhouhao <zhouhao@cn.fujitsu.com>
@zhouhao3
Copy link
Author

zhouhao3 commented Nov 3, 2017

rebased.

@zhouhao3
Copy link
Author

Closed because the code has been deleted.

@zhouhao3 zhouhao3 closed this Dec 13, 2017
@zhouhao3 zhouhao3 deleted the namespec-fix branch December 13, 2017 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wking wking wking left review comments

@Mashimiao Mashimiao Mashimiao left review comments

@liangchenye liangchenye liangchenye left review comments

Assignees
No one assigned
Labels
waiting spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhouhao3 @wking @liangchenye @Mashimiao