JSON objects are easier to parse/manipulate

[timthelion]

Don't use strings when you can use dictionaries/objects. JSON objects are trivial to parse and manipulate, unlike strings. String parsing is the #1 cause of security bugs, so if it can be trivially avoided, then why not ;)

[vbatts]

understandable, but not the format for mount. This options string value is the arg provided to mount.
Granted, this arg is not order dependent, so concatenating them for a single string joined by commas is not awful, but leaving it as the string to be provided removes a moving-part of processing where other issues could be introduced.
I'm on the fence about this.

[timthelion]

@vbatts You write "but leaving it as the string to be provided removes a moving-part of processing where other issues could be introduced." That "moving part" is a simple string concatination. That string computation is a one liner in most programming languages. If you use a string and for some reason that string had to be modified (to add, remove, or change on option) then the moving part would become something complex.

Furthermore, I argue that this object WILL need to be modified at run time. In the examples, we see gid being specified as well as the size of the tmpfs. The idea of parsing is not merely theoretical. So the choice is whether we want to parse that string or whether we want to concatinate a JSON object. Concatination is the obvious choice, especially since the once parsed string must be reconcatinated anyways.

[vbatts]

Fair. I just prefer fewer moving parts.

Then the formating should likely be standardized. https://github.com/opencontainers/specs/pull/120/files#diff-695bac0ce528e5c6765a6666fcccc425R30 has mixed type values. These integers might should stay as strings, so there is not a type handling assumption for any given json library, and then string concat and join.

[mrunalp]

IIRC, we did have a similar discussion on runc/libcontainer and had voted to keep it as it is.

[timthelion]

@mrunalp but you were wrong ;) .

Joking aside, I think that your comment shouldn't have much weight in this current discussion. What arguments were raised against using an object? Lets try to make decisions based on the techincal merit of arguments and not past concensus.

@vbatts I have changed gid to use a string.

Along the same lines as only using strings as values, I was thinking that it might make sense to use nil rather than true for options that are not set to anything. What do you think?

[vbatts]

same here. this is a bool, but if rw expects a string value of true, then this might as well be "true"

[timthelion]

It doesn't expect to be true, it expects to be rw. The pattern is, if there is a string on the right hand side(RHS) then the syntax is item=string and if it is true then the syntax is item without an equals sign. That's why I suggested using nil rather than true.

[vbatts]

oh hurr. you're right. i wasn't even reading that as just rw. Would these be better as an array of strings then?

[timthelion]

Well I thought about that too, that would make:

["nosuid","noexec","newinstance","ptmxmode=0666","mode=0620","gid=5"]

And the code to change the gid searches through that list for a string that starts with "gid=" and replaces that string. Not too bad, but still a form of parsing.

[vbatts]

fair, but I think this is more acceptable though as the mount -o ... is required as a comma delimited list. So this would put the formating as the greatest common denominator.

[timthelion]

@vbatts I've updated the PR.

[vbatts]

nice nice.
I wonder if verbiage would be good to show the alignment, at least for linux/*nix is matching to mount and fstab option formating.

Also, I presume there could be similar for windows, but i'm unfamiliar there.

[dqminh]

i think this looks good. the array version is much better than object version.

Also documentation for options needs to be changed, not a string anymore.

[timthelion]

@dqminh oops, thanks.

[philips]

This is fine with me. The primary two folks wanting a flat options string were @crosbymichael and @LK4D4 IIRC.

[mrunalp]

If we are going to change this, then might as well make it so there is no parsing required if possible.
I can't find any mount option that requires the value to be set to true.

[vbatts]

@mrunalp that was the point in making it not a dictionary. mount options are notkey=value, they are just a comma delimited list.

[mrunalp]

@vbatts Yes, but creating this new array still allows for items in the list to be key=value and that requires parsing the item to make sure it is correct which is only slightly better than what it is today.

A dictionary could be converted into mount options in a single loop and there is no parsing necessary.
To handle keys like newinstance we can require their value to be true like discussed earlier in the thread. Also, a validation tool could then check for invalid mount options without having to parse each item in the list.

[vbatts]

the discussed true was in regards to: what =value to provide in a dictionary, for a key that requires no value, like rw

[mrunalp]

@vbatts Yep, that's what I was referring to as well.

[vbatts]

That seems like sloppy semantics though. How is {"newinstance":false} expected to be handled?

[vbatts]

And a technical side note, a few mount option parsers handle overrides where the right-most wins. So order matters. This is assured in lists, and not in dictionary maps.

[mrunalp]

@vbatts I am convinced but don't see what this PR buys then. However, I will stay out and go with whatever is decided :)

[vbatts]

Haha. That circles back to my first concern. I could still be on the fence,
but figure we mimic the least common denominator for requirements.
On Aug 27, 2015 5:42 PM, "Mrunal Patel" notifications@github.com wrote:

@vbattshttps://github.com/vbatts I am convinced but don't see what
this PR buys then. However, I will stay out and go with whatever is decided
:)

—
Reply to this email directly or view it on GitHub
#120 (comment).

[crosbymichael]

change fine for me, looks exactly the same now just more " ;)

[wking]

I am agnostic here, but thought I'd link to #31, since that was the last push to change the mount format (although it was pushing for a less-structured model, and this PR aims for more structure). In any case (and maybe in this PR since it touches those lines), I think we should remove the no-options entries entirely (old ‘"options": ""’ and new ‘"options": []’). The field is optional, so why bother giving an empty value?

[crosbymichael]

using the list LGTM

[LK4D4]

LGTM too

[vbatts]

LGTM