Remove string pointers

[mrunalp]

An empty string works as null value for strings so we don't need string pointers.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

[crosbymichael]

LGTM

[duglin]

Not sure if this is an issue or not but wanted to mention it in case there's a hidden issue.... but the pointer allowed for the receiving side to know when the sender didn't mention the property at all vs they specified "". This is useful when you need to know when the user wants the default value (not mentioned) vs they explicitly want an empty string as the value. Just thought I'd mention it.

[duglin]

none of those fields look like they would have an issue with this, but others would know for sure.

[wking]

On Thu, Jan 12, 2017 at 10:06:22AM -0800, Mrunal Patel wrote: An empty string works as null value for strings so we don't need string pointers.

You'll also want the schema change from #548. And probably a note in the style.md section to avoid folks re-introducing *string in the future.

[wking]

On Thu, Jan 12, 2017 at 10:17:00AM -0800, Doug Davis wrote: This is useful when you need to know when the user wants the default value (not mentioned) vs they explicitly want an empty string as the value.

This had been the initial motivation for consistently using pointers for optional settings [1]. But we keep poking holes in that guideline, so one more hole for strings is probably not bit deal ;). And none of the properties being touched here look like users would want to explicitly set an empty string. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc3/style.md#optional-settings-should-have-pointer-go-types

[mrunalp]

@duglin Thanks! I think cgroupPath should be fine as empty could mean let runtime handle it. For cpuset cpus/mems, I see that when we create a new cgroup it is empty :/ @crosbymichael @hqhq WDYT we should do about that?

[wking]

While we're touching style.md (assuming that happens in this PR), GitHub's shifted their comment naming around and the initial source for preferring pointers for optional settings is [1] (where @mrunalp argues for *string for Cpus ;). I'm personally a fan of *string for those reasons, but I'm not a maintainer, and it's not really a big deal either way. [1]: #233 (comment)

[crosbymichael]

I think those are fine to keep as a non pointer because empty values on the cpus and mems is invalid

[wking]

I think those are fine to keep as a non pointer because empty values on the cpus and mems is invalid

Actually, an empty value for cpus seems to be legal:

# cd /sys/fs/cgroup/cpuset
# mkdir bob
# cat bob/cpuset.cpus

# echo 1 >bob/cpuset.cpus
# cat bob/cpuset.cpus
1
# echo >bob/cpuset.cpus
# cat bob/cpuset.cpus

And the docs have:

If hotplug functionality is used to remove all the CPUs that are currently assigned to a cpuset, then all the tasks in that cpuset will be moved to the nearest ancestor with non-empty cpus.

So we may want to retain the ability to explicitly write an empty string to cpuset.cpus.

[crosbymichael]

@wking its incorrect

[wking]

@wking its incorrect

The kernel allows it (and for cpuset.mems), at least with my vanilla 4.7. If the OCI position is that the kernel is wrong and the empty string is not something that you should be able to specify, it's probably worth explaining the reasoning behind that position.

[crosbymichael]

@wking your example only works because you don't have any processes in the cgroup

[wking]

@wking your example only works because you don't have any processes in the cgroup

Ah, that makes sense then. We're more restricted than the kernel (not allowing empty strings for cpuset.cpus or cpuset.mems) because we are always configuring cgroups immediately before adding a process to that cgroup. So I'm back to having no fundamental concerns about this approach for these settings.

868e631 is still missing JSON Schema and style.md updates, though. If @mrunalp doesn't want to address that himself, I can file a fixup PR against this one or a followup PR if/when this one lands.

[crosbymichael]

You know, it gets really old having to argue with you about everything, all the time. Can ya stop plz? I'm getting really frustrated.

[wking]

You know, it gets really old having to argue with you about everything…

Dropping the ability to set an empty-string value seems like something that should be backed up with at least a line or two of motivation for why you don't need that ability. I'm fairly sure someone else besides me will wonder why we don't support the empty-string cpus/mems values that the kernel allows in general, and now we can point them to your comment to clear them up.

I've filed the remaining changes I'd like to see against this branch in mrunalp#1 in case that helps.

[hqhq]

LGTM

@wking You can file schema changes as a followup PR, thanks.