Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jandubois opened this issue May 22, 2023 · 1 comment
Open

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

jandubois opened this issue May 22, 2023 · 1 comment

Comments

@jandubois
Copy link
Contributor

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
Copy link
Contributor Author

jandubois commented May 22, 2023

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)
                }

@AkihiroSuda AkihiroSuda changed the title clonefile() fails if the target already exists [Darwin] clonefile() fails if the target already exists May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant