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.

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

validation: add args validation for process #116

Closed

Conversation

Mashimiao
Copy link

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

if filepath.IsAbs(command) {
cmdPath := path.Join(rootfs, command)
if _, err := os.Stat(cmdPath); err != nil {
logrus.Fatalf("Cannot find the command path %q", cmdPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is too strict. Perhaps the user intends to copy the executable into the right location before running start (see earlier discussion here). Or maybe the user will bind-mount the command in from somewhere else. Anyhow, I think it's to complicated to try and make this a validation-time check, and we should just leave it to the runtime/kernel to error out if the command isn't there at execve-time (or whatever syscall the runtime is triggering with ‘start’).

Copy link
Contributor

Choose a reason for hiding this comment

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

See also my notes here on the current spec phrasing being inconsistent. The in-flight opencontainers/runtime-spec#427 should help make the specification more self-consistent.

Copy link
Author

Choose a reason for hiding this comment

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

@wking Thanks for you information. I think you are right. I will remove code which checks whether command exists or not
From runtime-sepc, it seems args is required not optional. So I think checking whether args is empty is necessary.

Copy link
Author

Choose a reason for hiding this comment

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

sorry, It seems no need to check args is empty or not. I will close this PR.

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the add-process-args-validation branch from a390617 to 7778014 Compare June 22, 2016 01:25
@Mashimiao Mashimiao closed this Jun 22, 2016
@wking
Copy link
Contributor

wking commented Jun 22, 2016

On Tue, Jun 21, 2016 at 06:47:39PM -0700, Ma Shimiao wrote:

Closed #116.

Wait, what? I was pushing back on the executable lookup 1, but I
think the current spec wording unambiguously requires at least one
entry in the process.args array 2, and after my suggested spec
changes that will still be the case 3.

@Mashimiao
Copy link
Author

@wking there is a function checkMandatoryUnit in validation.go can check args wheter should be empty or not. So, I think there is no need to check it again.
Am I right?

@wking
Copy link
Contributor

wking commented Jun 22, 2016

On Tue, Jun 21, 2016 at 09:58:49PM -0700, Ma Shimiao wrote:

there is a function checkMandatoryUnit in validation.go can check
args whether should be empty or not. So, I think there is no need to
check it again.

Ah, you're right:

$ ./ocitools generate
$ CONFIG=$(jq 'del(.process.args)' config.json)
$ echo "${CONFIG}" >config.json
$ ./ocitools validate
FATA[0000] Mandatory information missing: ['Process.Args' should not be empty.].

So I agree that this PR wasn't needed.

@Mashimiao Mashimiao deleted the add-process-args-validation branch November 14, 2016 09:43
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.

2 participants