Allow + in container ID

[pankit]

Signed-off-by: pankit thapar pankit@umich.edu

[mikebrow]

4 prs for a one char edit? :-) I've been there

Here you go for next time when there is a merge conflict:

<< from your branch directory >>>
git checkout master
git fetch upstream master --tags
git merge upstream/master
git push origin master
<<< now your master on git hub and local is up to date with the current master>>>
git checkout yourBranchName
git rebase master
<<< follow on screen instructions... if there's an issue rebasing you might need to open up the file and fix it  you'll see >>>>>>> arrows indicating what you need to fix up... then you continue your rebase using the on screen instructions >>>>
<<< run make / make test and check your fix etc.>>>
git add <the files you edited>
git commit -s --amend 
git push -f origin yourBranchName
<< go back into your PR on line and you will now see no merge conflicts!! >>

[pankit]

@mikebrow thanks for the guidance. I missed signoff once time and then it just went down the hill from there on. :)

[mikebrow]

Sure np... I've done the same before :-) If you forget to sign ... I believe you should be able to just sign and force push to update it...

git commit -s --amend
git push -f origin yourBranchName

[marcosnils]

@mikebrow IIUC containerId format should be valid path names as the state needs to be persisted in disk. It's totally valid to have a file called "something+stuff" so I guess this change makes sense.

[mikebrow]

Limiting to ascii letters might be wrong as well... while thinking about it.

[wking]

On Tue, Mar 22, 2016 at 09:57:43AM -0700, Marcos Nils wrote:

… containerId should be valid path names as the state needs to be
persisted in disk.

Even this requirement was lifted in opencontainers/runtime-spec#225.

[mikebrow]

Yes... I think Marco meant runc might need some code if the id can't be used as a path name..

[marcosnils]

@mikebrow exactly.

[pankit]

@mikebrow I added just the + because I have a use case where the foo+abc is used as a dir name and I think we should allow the containers to be named following the same rules as file/dir names.
Yes, I agree with you that we should allow more chars that fall into this category.

[mikebrow]

Hmm.. thinking out loud...

1. do a touch cid to see if the OS supports it as a filename and report otherwise
2. figure out how to the fix the expression for the plethora of unicode chars that should also be valid
3. hash the passed in cid
4. do nothing...

[wking]

On Tue, Mar 22, 2016 at 10:27:12AM -0700, Mike Brown wrote:

Yes... I think Marco meant runc might need some code if the id can't
be used as a path name..

Like percent-encoding 1? Or just base64-encoding all container IDs
before using them as filenames 2?

[wking]

On Tue, Mar 22, 2016 at 10:37:47AM -0700, Mike Brown wrote:

3. hash the passed in cid

This probably isn't what you want, since runC's ‘list’ walks the set
of available containers. There's no list-like command in the OCI
spec, but something like base64 would give you filesystem-safe names
that are still decodable.

[mikebrow]

@wking yes.... and if hashed could fix list to get the cid from the state, after I store the original id in it... But yes your idea would also work.

[mikebrow]

Issue opened ... #676 to address this secondary discussion

[wking]

On Tue, Mar 22, 2016 at 10:41:44AM -0700, Mike Brown wrote:

… if hashed could fix list to get the cid from the state, after I
store the original id in it…

Good point, and hashing has nice sharding properties (I'm not sure how
far runC's registry is intended to scale). Either way, it will become
a runC implementation detail, and not a limitation that's exposed to
the runtime caller.

[pankit]

@mikebrow should we close this PR or let it go through while #676 is implemented?

[mikebrow]

No need to close this one unless someone comes along and says we should not have '+' chars in cid names.

[crosbymichael]

LGTM

[LK4D4]

LGTM