fix Bug 1688041 - podman image save removes existing image

[QiWang19]

fix Bug 1688041 - podman image save removes existing image

docker doesn't return error docker-archive:img1: docker-archive doesn't support modifying existing images
docker save can replace the existing file.
podman save should overwrite the existing file.
Signed-off-by: Qi Wang [email protected]

[rhatdan]

@QiWang19 Could you update the commit with a message similar to what you put into the git message, with spelling corrected.

[mtrmac]

I can’t see how this PR addresses the linked bug.

The linked bug says:

Actual results:
After podman rejects to modify the existing image, image is also removed.

Expected results:
After podman rejects to modify the existing image, image stays in place without modifications.

This PR does not remove any code that could remove an image; OTOH it does remove the code that rejects modification of an image, which is presumed by the “expected results” to continue to exist.

https://github.com/containers/libpod/blob/41019f747280ba2c47df2ce6f83ddebfc2000745/libpod/image/image.go#L1155 seems to be the actual cause.

[QiWang19]

@rhatdan docker directly overwrite the existing image. Should I match with docker or make it as the expected result?

https://github.com/containers/libpod/blob/41019f747280ba2c47df2ce6f83ddebfc2000745/libpod/image/image.go#L1155 removes the existing or created file no matter what kind of error occurs. I think another way to fix this is to not remove when the error is docker-archive:img1: docker-archive doesn't support modifying existing images

[mtrmac]

docker directly overwrite the existing image. Should I match with docker or make it as the expected result?

*shrug* I don’t have a strong opinion. @cyphar originally wrote that code in #148 , he may have a preference.

(Noting also the Path.wipe(filename) in https://github.com/SUSE/kiwi/pull/1013/files#diff-9c91fd11fc96f62d60e3a950087f948fR109 I’ve just happened to notice; overwrite may well be useful. OTOH now that the code does not overwrite, changing it to overwrite may break something that relies on the current behavior.)

[mtrmac]

https://github.com/containers/libpod/blob/41019f747280ba2c47df2ce6f83ddebfc2000745/libpod/image/image.go#L1155 removes the existing or created file no matter what kind of error occurs. I think another way to fix this is to not remove when the error is docker-archive:img1: docker-archive doesn't support modifying existing images

… and when the error is “I/O error reading the input“ and when getPolicyContext fails and… … … many other possible failures that could happen somewhere on the call stack. It’s just not safe enough to delete files unless the code knows that it is safe.

(It might make sense for docker-archive: to delete the output files on error (i.e. if Commit is never called) if it knows that it has created a new file. But it’s not obviously the right thing to do, notably it could make it more difficult to diagnose the issue in some cases — and I don’t think deleting partial output files is generally a common/assumed behavior.

See also #313 which allows writing to a named pipe or a special device: such files should not be deleted on error in any case, and once data is sent to a stream, it can’t be unsent.)

[mheon]

From the perspective of a consumer of the c/image API, I would really prefer that we not delete existing files here - if we end up deciding to go that course in Podman, we can detect that the file exists and remove it there, without locking every tool that uses c/image into that potentially undesirable behavior.