validation: add args validation for process#116
validation: add args validation for process#116Mashimiao wants to merge 1 commit intoopencontainers:masterfrom
Conversation
validate.go
Outdated
| 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) |
There was a problem hiding this comment.
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’).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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>
a390617 to
7778014
Compare
|
On Tue, Jun 21, 2016 at 06:47:39PM -0700, Ma Shimiao wrote:
Wait, what? I was pushing back on the executable lookup 1, but I |
|
@wking there is a function |
|
On Tue, Jun 21, 2016 at 09:58:49PM -0700, Ma Shimiao wrote:
Ah, you're right: $ ./ocitools generate So I agree that this PR wasn't needed. |
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com