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: add mount type check #119

Merged
merged 1 commit into from
Jul 13, 2016
Merged

validate: add mount type check #119

merged 1 commit into from
Jul 13, 2016

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

wking
wking reviewed Jun 23, 2016
View reviewed changes
validate.go
}
}

supportedTypes["nfs"] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I see an nfs entry in my /proc/filesystems (and an nfs4 entry). Why are you adding it by hand here?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, it seems it's really no need to add.

@wking
Copy link
Contributor

wking commented Jun 24, 2016

Also related to this and the ‘bind’ type is that mount.type might
become optional (opencontainers/runtime-spec#470). In ccon, bind
mounts are handled with explict MS_BIND (and MS_REC) flags 1.

@Mashimiao Mashimiao force-pushed the add-mount-type-check branch 2 times, most recently from 8550459 to 26cfdc4 Compare June 24, 2016 06:53
wking
wking reviewed Jun 27, 2016
View reviewed changes
man/ocitools-validate.1.md
@@ -21,6 +21,9 @@ Validate an OCI bundle
**--hooks**
Check specified hooks exist and are executable on the host.

**--host-specific**
Check host specified configs.
Copy link
Contributor

Choose a reason for hiding this comment

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

“specified” → “specific”. And I'd add more context like:

By default, validation only tests for compatibility with a hypothetical host. With this flag, validation will also run more specific tests to see whether the current host is capable of launching a container from the configuration. For example, validating a compliant Windows configuration on a Linux machine will pass without this flag ("there may be a Windows host capable of launching this container"), but will fail with it ("this host is not capable of launching this container").

wking
wking reviewed Jun 27, 2016
View reviewed changes
man/ocitools-validate.1.md
For example, validating a compliant Windows configuration on a Linux machine
will pass without this flag ("there may be a Windows host capable of
launching this container"), but will fail with it ("this host is not capable
of launching this container").
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there's trailing whitespace here (in case we care in this repository; I don't if we do).

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Thanks.

@wking
Copy link
Contributor

wking commented Jun 27, 2016

32803b5 looks good to me, excepting a possible whitespace nit 1 and
a warning for Linux configs when --host-specific is not set 2. I'm
fine punting checkPlatform changes to a follow-up PR 3.

@Mashimiao
Copy link
Author

ping @liangchenye , @mrunalp

@liangchenye
Copy link
Member

LGTM
the --host-specific looks great to me 👍

wking
wking reviewed Jun 28, 2016
View reviewed changes
validate.go
if hostCheck {
f, err := os.Open("/proc/filesystems")
if err != nil {
return supportedTypes, err
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably return nil, err. There's not much point in returning an empty map, and the caller will bail when it sees the error anyway.

Copy link
Author

Choose a reason for hiding this comment

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

On 06/28/2016 11:21 AM, W. Trevor King wrote:

This should probably return |nil, err|. There's not much point in returning an empty map, and the caller will bail when it sees the error anyway.
fixed. Thanks.

Ma Shimiao
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)

validate: add mount type check
f1ff4d0 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@wking
Copy link
Contributor

wking commented Jun 28, 2016 via email

@Mashimiao
Copy link
Author

ping @mrunalp

@wking wking mentioned this pull request Jul 9, 2016
Open
@Mashimiao
Copy link
Author

ping @mrunalp

@mrunalp
Copy link
Contributor

mrunalp commented Jul 13, 2016

LGTM

@mrunalp mrunalp merged commit 3c402c7 into opencontainers:master Jul 13, 2016
wking added a commit to wking/ccon that referenced this pull request Jul 23, 2016
@wking
WIP: ccon-oci: (r)bind handling
e867fc3 
See [1,2].

[1]: opencontainers/runtime-spec#443 (comment)
[2]: opencontainers/runtime-tools#119 (comment)
@wking wking mentioned this pull request Aug 13, 2016
Closed
@Mashimiao Mashimiao deleted the add-mount-type-check branch November 14, 2016 09:43
wking added a commit to wking/ccon that referenced this pull request Feb 26, 2018
@wking
WIP: ccon-oci: (r)bind handling
a50a751 
See [1,2].

[1]: opencontainers/runtime-spec#443 (comment)
[2]: opencontainers/runtime-tools#119 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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