containerd / continuity

A transport-agnostic, filesystem metadata manifest system
https://containerd.io
Apache License 2.0
137 stars 66 forks source link

[Darwin] clonefile() fails if the target already exists #226

Open jandubois opened 1 year ago

jandubois commented 1 year ago

This breaks the expected copyFile semantics, which should truncate an existing file instead of throwing an error.

We could pro-actively remove the target:

 func copyFile(target, source string) error {
+       if err := os.Remove(target); err != nil {
+               if !errors.Is(err, unix.ENOENT) {
+                       return fmt.Errorf("removing copyFile target failed: %w", err)
+               }
+       }
        if err := unix.Clonefile(source, target, unix.CLONE_NOFOLLOW); err != nil {
                if !errors.Is(err, unix.ENOTSUP) {
                        return fmt.Errorf("clonefile failed: %w", err)

But I'm not sure if this could become a problem when clonefile is not supported, or when the copy operation is cross-device, and we still fall back to copyFile.

I'm happy to create a PR with the patch above, but would like confirmation first that this would indeed be the correct approach.

jandubois commented 1 year ago

Maybe a more conservative approach to only remove the target when clonefile() returns EEXIST is safer (but adds an early success-return in the middle of the code):

 func copyFile(target, source string) error {
        if err := unix.Clonefile(source, target, unix.CLONE_NOFOLLOW); err != nil {
+               if errors.Is(err, unix.EEXIST) {
+                       if err = os.Remove(target); err != nil {
+                               return fmt.Errorf("removing clonefile target failed: %w", err)
+                       }
+                       if err = unix.Clonefile(source, target, unix.CLONE_NOFOLLOW); err == nil {
+                               return nil
+                       }
+               }
                if !errors.Is(err, unix.ENOTSUP) {
                        return fmt.Errorf("clonefile failed: %w", err)
                }