Skip to content

Conversation

@wking
Copy link
Contributor

@wking wking commented Apr 12, 2017

I expect the (undocumented) intention here is to iterate through names and call seccomp_rule_add(3) or similar for each name. In that case, an empty names makes the whole syscall entry a no-op, and with this commit we can warn users who are validating such configs.

If, on the other hand, we were comfortable with no-op syscall entries, we'd want to make names OPTIONAL.

Warning folks who accidentally empty (or don't set) names seems more useful to me, and doesn't restrict the useful config space, so that's what I've gone with in this commit.

minItems is documented here, and there is an example of its use here:

  "options": {
    "type": "array",
    "minItems": 1,
    "items": { "type": "string" },
    "uniqueItems": true
  },

Part of #762.

…ames

I expect the (undocumented) intention here is to iterate through
'names' and call seccomp_rule_add(3) or similar for each name.  In
that case, an empty 'names' makes the whole syscall entry a no-op, and
with this commit we can warn users who are validating such configs.

If, on the other hand, we were comfortable with no-op syscall entries,
we'd want to make 'names' OPTIONAL.

Warning folks who accidentally empty (or don't set) 'names' seems more
useful to me, and doesn't restrict the useful config space, so that's
what I've gone with in this commit.

minItems is documented in [1], and there is an example of its use in
[2]:

  "options": {
    "type": "array",
    "minItems": 1,
    "items": { "type": "string" },
    "uniqueItems": true
  },

[1]: https://tools.ietf.org/html/draft-wright-json-schema-validation-00#section-5.11
[2]: http://json-schema.org/example2.html

Signed-off-by: W. Trevor King <[email protected]>
@wking
Copy link
Contributor Author

wking commented Apr 13, 2017

This is a breaking change (adding a new MUST-level restriction), so it would be good to get it into 1.0. Otherwise we'd have to delay this until 2.0.

Copy link

@dklyle dklyle left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@mrunalp
Copy link
Contributor

mrunalp commented Apr 26, 2017

LGTM

Approved with PullApprove

2 similar comments
@tianon
Copy link
Member

tianon commented Apr 26, 2017

LGTM

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Apr 26, 2017

LGTM

Approved with PullApprove

@vbatts vbatts merged commit c6bff91 into opencontainers:master Apr 26, 2017
@wking wking deleted the require-syscall-names branch April 26, 2017 16:08
@vbatts vbatts mentioned this pull request Jul 5, 2017
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.

5 participants