fix cp none exists dest path ends with '/' #4101

QiWang19 · 2019-09-24T15:40:58Z

close #3894
This patch will let podman cp return 'no such file or directory' error if DEST_PATH does not exist and ends with / when copying file.

$ podman run -d --name cpc alpine sh -c 'sleep 999'
$ podman exec cpc sh -c 'mkdir /tmp/d1;ln -s /tmp/nonesuch1 /tmp/d1/x'
$ podman exec cpc sh -c 'mkdir /tmp/d2;ln -s /tmp/nonesuch2 /tmp/d2/x'
$ podman exec cpc sh -c 'mkdir /tmp/d3'
$ podman exec cpc ls -lR /tmp
/tmp:
total 12
drwxr-xr-x    2 root     root          4096 Sep 24 14:45 d1
drwxr-xr-x    2 root     root          4096 Sep 24 14:45 d2
drwxr-xr-x    2 root     root          4096 Sep 24 14:45 d3

/tmp/d1:
total 0
lrwxrwxrwx    1 root     root            14 Sep 24 14:45 x -> /tmp/nonesuch1

/tmp/d2:
total 0
lrwxrwxrwx    1 root     root            14 Sep 24 14:45 x -> /tmp/nonesuch2

/tmp/d3:
total 0

$ podman cp /tmp/myfile cpc:/tmp/d1/x
$ podman cp /tmp/myfile cpc:/tmp/d2/x/
Error: failed to get stat of dest path /home/qiwan/.local/share/containers/storage/overlay/82311cd5e4e276073e44ad65eaeec65a048a3141d447d4b46393b1c3fac64168/merged/tmp/nonesuch2: stat /home/qiwan/.local/share/containers/storage/overlay/82311cd5e4e276073e44ad65eaeec65a048a3141d447d4b46393b1c3fac64168/merged/tmp/nonesuch2: no such file or directory
$ podman cp /tmp/myfile cpc:/tmp/d3/x
$ podman exec cpc ls -lR /tmp
/tmp:
total 16
drwxr-xr-x    2 root     root          4096 Sep 24 14:45 d1
drwxr-xr-x    2 root     root          4096 Sep 24 14:45 d2
drwxr-xr-x    2 root     root          4096 Sep 24 14:46 d3
-rw-rw-r--    1 root     root             3 Sep 24 14:07 nonesuch1

/tmp/d1:
total 0
lrwxrwxrwx    1 root     root            14 Sep 24 14:45 x -> /tmp/nonesuch1

/tmp/d2:
total 0
lrwxrwxrwx    1 root     root            14 Sep 24 14:45 x -> /tmp/nonesuch2

/tmp/d3:
total 4
-rw-rw-r--    1 root     root             3 Sep 24 14:07 x

Signed-off-by: Qi Wang qiwan@redhat.com

QiWang19 · 2019-09-24T15:41:34Z

@TomSweeneyRedHat @edsantiago @mheon PTAL

edsantiago · 2019-09-24T15:49:21Z

Don't forget to update test/system/065-cp.bats

TomSweeneyRedHat · 2019-09-24T16:48:29Z

cmd/podman/cp.go

@@ -329,6 +329,9 @@ func copy(src, destPath, dest string, idMappingOpts storage.IDMappingOptions, ch

 	destfi, err := os.Stat(destPath)
 	if err != nil {
+		if strings.HasSuffix(dest, string(os.PathSeparator)) {


looks like the same error message, can you do a single if with an "or" statement?

TomSweeneyRedHat · 2019-09-24T16:50:47Z

@QiWang19 some of the tests aren't too happy.

edsantiago · 2019-09-24T18:59:41Z

test/system/065-cp.bats

@@ -149,7 +149,7 @@ load helpers
    is "$output" "" "output from podman cp 1"

    run_podman cp --pause=false $srcdir/$rand_filename2 cpcontainer:/tmp/d2/x/
-    is "$output" "" "output from podman cp 3"
+    is "$output" "Error: failed to get stat of dest path .*stat.* no such file or directory" "cp returns error no such file or directory"


You need to change the expected exit status, e.g.:

run_podman 12345 cp --pause ...

...where 12345 is the expected exit status of the cp command (probably 1, but may be something else, see test logs)

@edsantiago thanks. 🙂 Do I still need keep this current change?

The check for "Error"? Yes.

Since changes are needed, please also change the test description:

- "cp returns error no such file or directory" + "cp will not create nonexistent destination directory"

QiWang19 · 2019-09-25T14:37:08Z

@TomSweeneyRedHat @edsantiago @mheon PTAL

edsantiago

Sorry for replying so late; there was too much churn yesterday.

edsantiago · 2019-09-25T14:51:03Z

test/system/065-cp.bats

@@ -148,8 +148,8 @@ load helpers
    run_podman cp --pause=false $srcdir/$rand_filename1 cpcontainer:/tmp/d1/x


The line immediately above this one, #147, is a comment that is now incorrect. Please change it:

- # Note that the second has a trailing slash; this will trigger mkdir + # Note that the second has a trailing slash, implying a directory. + # Since that destination directory doesn't exist, the cp will fail

Sorry for not catching this sooner.

edsantiago · 2019-09-25T14:52:36Z

test/system/065-cp.bats

@@ -149,7 +149,7 @@ load helpers
    is "$output" "" "output from podman cp 1"

    run_podman cp --pause=false $srcdir/$rand_filename2 cpcontainer:/tmp/d2/x/
-    is "$output" "" "output from podman cp 3"
+    is "$output" "Error: failed to get stat of dest path .*stat.* no such file or directory" "cp returns error no such file or directory"


Since changes are needed, please also change the test description:

- "cp returns error no such file or directory" + "cp will not create nonexistent destination directory"

edsantiago · 2019-09-25T14:53:09Z

test/system/065-cp.bats

@@ -163,8 +163,8 @@ load helpers

    # In the second case, podman creates a directory nonesuch2, then
    # creates a file with the same name as the input file. THIS IS WEIRD!


Please also fix these comments.

edsantiago · 2019-09-25T15:01:53Z

test/system/065-cp.bats

@@ -163,8 +163,8 @@ load helpers

    # In the second case, podman creates a directory nonesuch2, then
    # creates a file with the same name as the input file. THIS IS WEIRD!
-    run_podman exec cpcontainer cat /tmp/nonesuch2/$rand_filename2
-    is "$output" "$rand_content2" "cp creates destination dir and file"
+    run_podman 1 exec cpcontainer cat /tmp/nonesuch2/$rand_filename2


Since podman behavior has changed, this is no longer a great test: it simply checks that the destination file doesn't exist - but it is conceivable (albeit improbable) that podman created the destination directory before exiting. That would be very bad. A much better test would now be:

# cp into nonexistent directory should not mkdir said directory run_podman 1 exec cpcontainer test -e /tmp/nonesuch2

This test is much more useful.

edsantiago · 2019-09-25T15:07:01Z

test/system/065-cp.bats

@@ -221,4 +221,4 @@ function teardown() {
    basic_teardown
 }

-# vim: filetype=sh
+# vim: filetype=sh


Please fix. (You removed the closing newline. Please add it back).

close containers#3894 This patch let podman cp return 'no such file or directory' error if DEST_PATH does not exist and ends with / when copying file. Signed-off-by: Qi Wang <qiwan@redhat.com>

TomSweeneyRedHat · 2019-09-25T17:16:10Z

Code changes LGTM, I'll let you and @edsantiago work through the testing comments. Some of the tests aren't happy ATM.

edsantiago · 2019-09-25T17:33:12Z

Latest change (0144c37) LGTM.

@TomSweeneyRedHat the CI "failures" are the same as yesterday, i.e. false alarms

mheon · 2019-09-25T17:39:47Z

/approve
LGTM

openshift-ci-robot · 2019-09-25T17:39:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, mheon, QiWang19

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:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

QiWang19 · 2019-09-25T20:39:52Z

Needs lgtm label. 🏷️

mheon · 2019-09-25T21:29:00Z

/lgtm

openshift-ci-robot requested review from umohnani8 and vrothberg September 24, 2019 15:41

openshift-ci-robot added the size/S label Sep 24, 2019

TomSweeneyRedHat reviewed Sep 24, 2019

View reviewed changes

QiWang19 force-pushed the nonexistdir branch from 8e5406a to bc106fe Compare September 24, 2019 17:26

openshift-ci-robot added size/M and removed size/S labels Sep 24, 2019

QiWang19 force-pushed the nonexistdir branch from bc106fe to 643a25c Compare September 24, 2019 17:31

openshift-ci-robot added size/S and removed size/M labels Sep 24, 2019

QiWang19 force-pushed the nonexistdir branch 2 times, most recently from 8a94a91 to 23c3962 Compare September 24, 2019 17:45

edsantiago reviewed Sep 24, 2019

View reviewed changes

QiWang19 force-pushed the nonexistdir branch from 23c3962 to 9e5f698 Compare September 24, 2019 20:25

openshift-ci-robot added size/M and removed size/S labels Sep 24, 2019

QiWang19 force-pushed the nonexistdir branch from 9e5f698 to d12be35 Compare September 25, 2019 13:46

edsantiago requested changes Sep 25, 2019

View reviewed changes

fix cp none exists dest path ends with '/'

0144c37

close containers#3894 This patch let podman cp return 'no such file or directory' error if DEST_PATH does not exist and ends with / when copying file. Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the nonexistdir branch from d12be35 to 0144c37 Compare September 25, 2019 16:48

edsantiago approved these changes Sep 25, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2019

openshift-ci-robot assigned mheon Sep 25, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2019

openshift-merge-robot merged commit 3ed265c into containers:master Sep 25, 2019

rh-atomic-bot mentioned this pull request Sep 25, 2019

fix bug copy to volume and bind mount #3769

Closed

QiWang19 deleted the nonexistdir branch June 26, 2020 15:10

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix cp none exists dest path ends with '/' #4101

fix cp none exists dest path ends with '/' #4101

QiWang19 commented Sep 24, 2019

QiWang19 commented Sep 24, 2019

edsantiago commented Sep 24, 2019

TomSweeneyRedHat Sep 24, 2019 •

edited

TomSweeneyRedHat commented Sep 24, 2019

edsantiago Sep 24, 2019

QiWang19 Sep 24, 2019

edsantiago Sep 24, 2019

QiWang19 Sep 24, 2019

edsantiago Sep 25, 2019

QiWang19 commented Sep 25, 2019

edsantiago left a comment

edsantiago Sep 25, 2019

edsantiago Sep 25, 2019

edsantiago Sep 25, 2019

edsantiago Sep 25, 2019

edsantiago Sep 25, 2019

TomSweeneyRedHat commented Sep 25, 2019

edsantiago commented Sep 25, 2019

mheon commented Sep 25, 2019

openshift-ci-robot commented Sep 25, 2019

QiWang19 commented Sep 25, 2019

mheon commented Sep 25, 2019

		@@ -148,8 +148,8 @@ load helpers
		run_podman cp --pause=false $srcdir/$rand_filename1 cpcontainer:/tmp/d1/x

		@@ -163,8 +163,8 @@ load helpers

		# In the second case, podman creates a directory nonesuch2, then
		# creates a file with the same name as the input file. THIS IS WEIRD!

fix cp none exists dest path ends with '/' #4101

fix cp none exists dest path ends with '/' #4101

Conversation

QiWang19 commented Sep 24, 2019

QiWang19 commented Sep 24, 2019

edsantiago commented Sep 24, 2019

TomSweeneyRedHat Sep 24, 2019 • edited

Choose a reason for hiding this comment

TomSweeneyRedHat commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Sep 25, 2019

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Sep 25, 2019

edsantiago commented Sep 25, 2019

mheon commented Sep 25, 2019

openshift-ci-robot commented Sep 25, 2019

QiWang19 commented Sep 25, 2019

mheon commented Sep 25, 2019

TomSweeneyRedHat Sep 24, 2019 •

edited