Support overlayfs path contains colon #3581
Conversation
|
@chenk008 thanks for the PR. Our CI system won't accept a PR without a new test added. Could you add a simple one, and your verification sample would work well, please? Otherwise the code LGTM. Although a small comment above your new function might be useful. |
|
@TomSweeneyRedHat Thanks for your suggestion. I am trying to find a way to add some test. I found only |
|
@chenk008 that would work for me, but I'm not sure if you need the |
|
@chenk008 Could you break your commit message into multiple lines. CI is failing because commit message is too long. |
chenk008
changed the title
chenk008
force-pushed
the
support_colon_in_overlay_path
branch
3 times, most recently
from
a85cb01
to
93dff78
Compare
|
@TomSweeneyRedHat Indeed the |
|
@flouthoc Thanks for your suggestion, I rewrited the commit message. |
chenk008
force-pushed
the
support_colon_in_overlay_path
branch
3 times, most recently
from
e8a2365
to
688b9fa
Compare
chenk008
changed the title
|
@chenk008 You need to rebase your merge to get rid of the merge commit. git rebase -i origin |
|
LGTM |
| val := SplitStringWithColonEscape(args.volume) | ||
| assert.Equal(t, val, args.expectedResult) | ||
| } | ||
| } |
|
LGTM |
chenk008
force-pushed
the
support_colon_in_overlay_path
branch
from
b4b2803
to
f3e30fd
Compare
|
@rhatdan Thanks for your suggestion. I have squashed commits. |
| @@ -105,8 +105,7 @@ func mountHelper(contentDir, source, dest string, _, _ int, graphOptions []strin | |||
| return mount, err | |||
| } | |||
| } | |||
|
|
|||
| overlayOptions = fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s,private", source, upperDir, workDir) | |||
| overlayOptions = fmt.Sprintf("lowerdir=%s,upperdir=%s,workdir=%s,private", escapeColon(source), upperDir, workDir) | |||
There was a problem hiding this comment.
Not scoped to this PR: but since we have a boilerplate for escaping we could also support escaping source and dest for other types not just overlay. No actionable on this comment just leaving here for note.
| // the colon is backslash-escaped | ||
| if idx-1 > 0 && str[idx-1] == '\\' { | ||
| sb.WriteRune(r) | ||
| } else { |
There was a problem hiding this comment.
Do we ever get to this else since all the escape colons are filtered by the first if.
There was a problem hiding this comment.
When we get a separator : without \, we will reach to the else and get a slice element.
In linux, directory can contains colon. Add support to mount path contains colon. buildah run --volume /root/a\\:b:/root/test:O Signed-off-by: chenk008 <kongchen28@gmail.com>
chenk008
force-pushed
the
support_colon_in_overlay_path
branch
from
f3e30fd
to
03186a3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chenk008, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: chenk008 kongchen28@gmail.com
What type of PR is this?
What this PR does / why we need it:
After patched this PR containers/podman#11912,
podman run --rootfs /root/a:b:O echo hellofailed with error messageBut overlayfs mount support path which contains colon, the below command works.
How to verify it
podman run --rootfs /root/a:b:O echo hellosucceeded to echo hello.Which issue(s) this PR fixes:
It is related to containers/podman#11913
Special notes for your reviewer:
Does this PR introduce a user-facing change?
None