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

Fix conversion of sysctl variable dots and slashes #3257

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

mengjiao-liu
Copy link

@mengjiao-liu mengjiao-liu commented Nov 1, 2021

Ref #3256

see

sysctl conversion function according to the linux sysctl conversion definition.

       Note that either "/" or "."  may be used as separators within
       sysctl variable names. If the first separator is a slash,
       remaining slashes and dots are left intact. If the first
       separator is a dot, dots and slashes are interchanged.
       "kernel.domainname=foo" and "kernel/domainname=foo" are
       equivalent and will cause "foo" to be written to
       /proc/sys/kernel/domainname. Either
       "net.ipv4.conf.enp3s0/200.forwarding" or
       "net/ipv4/conf/enp3s0.200/forwarding" may be used to refer to
       /proc/sys/net/ipv4/conf/enp3s0.200/forwarding. A glob glob(7)
       pattern may be used to write the same value to all matching keys.
       Keys for which an explicit pattern exists will be excluded from
       any glob matching. In addition, a key may be explicitly excluded
       from being set by any matching glob patterns by specifying the
       key name prefixed with a "-" character and not followed by "=",
       see SYNOPSIS.

see following webpages for more details:
https://man7.org/linux/man-pages/man8/sysctl.8.html
https://man7.org/linux/man-pages/man5/sysctl.d.5.html

Related to:

@mengjiao-liu
Copy link
Author

PTAL @AkihiroSuda @mrunalp

@AkihiroSuda AkihiroSuda added this to the 1.1.0 milestone Nov 1, 2021
@mengjiao-liu mengjiao-liu changed the title '/' is allowed as a separator in sysctl name Fix the conversion of sysctl variable dots and slashes Nov 1, 2021
AkihiroSuda
AkihiroSuda previously approved these changes Nov 1, 2021
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Left some nits.

Can you please separate out the package validate_test -> package validate change to a separate (first) commit?

libcontainer/configs/validate/validator.go Outdated Show resolved Hide resolved
libcontainer/configs/validate/validator.go Outdated Show resolved Hide resolved
libcontainer/configs/validate/validator.go Outdated Show resolved Hide resolved
libcontainer/configs/validate/validator.go Outdated Show resolved Hide resolved
libcontainer/configs/validate/validator_test.go Outdated Show resolved Hide resolved
libcontainer/configs/validate/validator_test.go Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor

LGTM except for a single nit, thank you @mengjiao-liu!

Also, this needs to be rebased due to #3261

@kolyshkin
Copy link
Contributor

Ah, and the signed-off-by needs to be added to the commits (see https://github.com/opencontainers/runc/pull/3257/checks?check_run_id=4100751081)

@mengjiao-liu
Copy link
Author

Rebased and signed-off-by. Thanks for reminding!

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@kolyshkin kolyshkin merged commit b647548 into opencontainers:master Nov 5, 2021
@mengjiao-liu mengjiao-liu changed the title Fix the conversion of sysctl variable dots and slashes Conversion of sysctl variable dots and slashes Nov 19, 2021
@mengjiao-liu mengjiao-liu changed the title Conversion of sysctl variable dots and slashes Fix conversion of sysctl variable dots and slashes Nov 30, 2021
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.

3 participants