-
Notifications
You must be signed in to change notification settings - Fork 597
config-linux,schema: fix FileMode description #1298
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
Conversation
config-linux.md
Outdated
| The path MAY be anywhere in the container filesystem, notably outside of `/dev`. | ||
| * **`major, minor`** *(int64, REQUIRED unless `type` is `p`)* - [major, minor numbers][devices] for the device. | ||
| * **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. | ||
| * **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. The value MUST be a decimal (not an octal) number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be within the scope of the spec of JSON, not of the runtime-spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but the issue is, any octal number without a prefix (such as 0, 0o etc.) is a valid decimal number as well, so I wanted to add some kind of a warning here, as everyone is accustomed to using octal numbers for mode.
Would a note look better?
| * **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. The value MUST be a decimal (not an octal) number. | |
| * **`fileMode`** *(uint32, OPTIONAL)* - file mode for the device. Note it is a decimal (not an octal) number. |
Or should we just drop this hunk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note looks better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Originally, the file mode was indeed written in octal (see e.g. commit 5273b3d), but it was found out later that JSON does not allow octal values so the examples were changed to decimal in commit ccf3a24, but the "typically an octal value" bit (added by commit cdcabde) remains. Change it to emphasize the fact that this is in decimal. Also, add a note to config-linux.md saying the same thing. Signed-off-by: Kir Kolyshkin <[email protected]>
Current spec allows decimal 512 as a maximum value for FileMode, which is octal 1000, meaning sticky bit is set and no rwx permissions for anyone (aka s---------). This does not make sense,the maximum value should be 511 (which is octal 777, aka -rwxrwxrwx). Signed-off-by: Kir Kolyshkin <[email protected]>
61f3527 to
9efd9f2
Compare
Originally, the file mode was indeed written in octal (see e.g. commit 5273b3d), but it was found out later that JSON does not allow octal values so the examples were changed to decimal in commit ccf3a24, but the "typically an octal value" bit (added by commit cdcabde) remains.
Change it to emphasize the fact that this is in decimal.
Add a note to config-linux.md saying the same thing.