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

Bug: copy only copies first 2GB of file #3679

Closed
p6rt opened this issue Feb 11, 2015 · 9 comments
Closed

Bug: copy only copies first 2GB of file #3679

p6rt opened this issue Feb 11, 2015 · 9 comments

Comments

@p6rt
Copy link

p6rt commented Feb 11, 2015

Migrated from rt.perl.org#123796 (status was 'resolved')

Searchable as RT123796$

@p6rt
Copy link
Author

p6rt commented Feb 11, 2015

From @colomon

This is perl6 version 2015.01-126-gf53a94d built on MoarVM version
2015.01-28-ga910556

copy("original.mpg".IO, "copy.mpg".IO)
True

colomon@​melissa​:~/temp$ ls -l *.mpg
-rw-rw-r-- 1 colomon colomon 2147479552 Feb 11 09​:51 copy.mpg
-rw-r--r-- 1 colomon colomon 4773115509 Feb 11 09​:50 original.mpg

--
Solomon Foster​: colomon@​gmail.com
HarmonyWare, Inc​: http://www.harmonyware.com

@p6rt
Copy link
Author

p6rt commented Feb 11, 2015

From @lizmat

On 11 Feb 2015, at 15​:51, Solomon Foster (via RT) <perl6-bugs-followup@​perl.org> wrote​:

# New Ticket Created by Solomon Foster
# Please include the string​: [perl #​123796]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=123796 >

This is perl6 version 2015.01-126-gf53a94d built on MoarVM version
2015.01-28-ga910556

copy("original.mpg".IO, "copy.mpg".IO)
True

colomon@​melissa​:~/temp$ ls -l *.mpg
-rw-rw-r-- 1 colomon colomon 2147479552 Feb 11 09​:51 copy.mpg
-rw-r--r-- 1 colomon colomon 4773115509 Feb 11 09​:50 original.mpg

Internally this is handled by the nqp​::copy op. I would bet that only the first 2G is copied because that’s what you can have in a signed 4byte int.

Liz

@p6rt
Copy link
Author

p6rt commented Feb 11, 2015

The RT System itself - Status changed from 'new' to 'open'

@p6rt
Copy link
Author

p6rt commented Feb 22, 2015

From @bdw

The bug here was that moar expected libuv sendfile to send all bytes that
are requested in one go, but it doesn't do that on linux (and perhaps other
platforms). Instead, 2G are sent at once. libuv sendfile differs a bit from
standard POSIX sendfile, but I think this patch fixes it.

Bart

2015-02-11 18​:41 GMT+01​:00 Elizabeth Mattijsen <liz@​dijkmat.nl>​:

On 11 Feb 2015, at 15​:51, Solomon Foster (via RT) <
perl6-bugs-followup@​perl.org> wrote​:

# New Ticket Created by Solomon Foster
# Please include the string​: [perl #​123796]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=123796 >

This is perl6 version 2015.01-126-gf53a94d built on MoarVM version
2015.01-28-ga910556

copy("original.mpg".IO, "copy.mpg".IO)
True

colomon@​melissa​:~/temp$ ls -l *.mpg
-rw-rw-r-- 1 colomon colomon 2147479552 Feb 11 09​:51 copy.mpg
-rw-r--r-- 1 colomon colomon 4773115509 Feb 11 09​:50 original.mpg

Internally this is handled by the nqp​::copy op. I would bet that only the
first 2G is copied because that’s what you can have in a signed 4byte int.

Liz

@p6rt
Copy link
Author

p6rt commented Feb 22, 2015

From @bdw

rt123796.patch
diff --git a/src/io/fileops.c b/src/io/fileops.c
index 2bdded8..3e446a0 100644
--- a/src/io/fileops.c
+++ b/src/io/fileops.c
@@ -118,41 +118,83 @@ MVMint64 MVM_file_stat(MVMThreadContext *tc, MVMString *filename, MVMint64 statu
     return r;
 }
 
-/* copy a file from one to another. */
-void MVM_file_copy(MVMThreadContext *tc, MVMString *src, MVMString *dest) {
+/* copy a file from one to another */
+void MVM_file_copy(MVMThreadContext *tc, MVMString *src, MVMString * dest) {
+    /* TODO: on Windows we can use the CopyFile API, which is probaly
+       more efficient, not to mention easier to use. */
     uv_fs_t req;
-    char *       const a = MVM_string_utf8_encode_C_string(tc, src);
-    const uv_file  in_fd = uv_fs_open(tc->loop, &req, (const char *)a, O_RDONLY, 0, NULL);
+    char * a, * b;
+    uv_file in_fd = -1, out_fd = -1;
+    MVMuint64 size, offset;
 
-    if (in_fd >= 0 && uv_fs_stat(tc->loop, &req, a, NULL) >= 0) {
-        char *       const b = MVM_string_utf8_encode_C_string(tc, dest);
-        const uv_file out_fd = uv_fs_open(tc->loop, &req, (const char *)b, O_WRONLY | O_CREAT | O_TRUNC, DEFAULT_MODE, NULL);
-        MVM_free(a);
+    a = MVM_string_utf8_encode_C_string(tc, src);
+    b = MVM_string_utf8_encode_C_string(tc, dest);
 
-        if (out_fd >= 0
-        && uv_fs_sendfile(tc->loop, &req, out_fd, in_fd, 0, req.statbuf.st_size, NULL) >= 0) {
-            MVM_free(b);
+    /* If the file cannot be stat(), there is little point in going any further. */
+    if (uv_fs_stat(tc->loop, &req, a, NULL) < 0)
+        goto failure;
+    size = req.statbuf.st_size;
 
-            if (uv_fs_close(tc->loop, &req, in_fd, NULL) < 0) {
-                uv_fs_close(tc->loop, &req, out_fd, NULL); /* should close out_fd before throw. */
-                MVM_exception_throw_adhoc(tc, "Failed to close file: %s", uv_strerror(req.result));
-            }
+    in_fd = uv_fs_open(tc->loop, &req, (const char *)a, O_RDONLY, 0, NULL);
+    if (in_fd < 0) {
+        goto failure;
+    }
 
-            if (uv_fs_close(tc->loop, &req, out_fd, NULL) < 0) {
-                MVM_exception_throw_adhoc(tc, "Failed to close file: %s", uv_strerror(req.result));
-            }
+    out_fd = uv_fs_open(tc->loop, &req, (const char *)b, O_WRONLY | O_CREAT | O_TRUNC, DEFAULT_MODE, NULL);
+    if (out_fd < 0) {
+        goto failure;
+    }
 
-            return;
+    offset = 0;
+    do {
+        /* sendfile() traditionally takes offset as a pointer argument
+         * used a both input and output. libuv deviates by making
+         * offset an integer and returning the number of bytes
+         * sent. So it is necessary to add these explicitly. */
+        MVMint64 sent = uv_fs_sendfile(tc->loop, &req, out_fd, in_fd, offset, size - offset, NULL);
+        if (sent < 0) {
+            goto failure;
         }
-        else
-            MVM_free(b);
+        offset += sent;
+    } while (offset < size);
+
+    /* Cleanup */
+    if(uv_fs_close(tc->loop, &req, in_fd, NULL) < 0) {
+        goto failure;
     }
-    else
-        MVM_free(a);
+    in_fd = -1;
 
-    MVM_exception_throw_adhoc(tc, "Failed to copy file: %s", uv_strerror(req.result));
+    if (uv_fs_close(tc->loop, &req, out_fd, NULL) < 0) {
+        goto failure;
+    }
+    out_fd = -1;
+
+    MVM_free(b);
+    MVM_free(a);
+    return;
+
+ failure: {
+        /* First get the error, since it may be overwritten further on. */
+        const char * error = uv_strerror(req.result);
+        /* Basic premise: dealing with all failure cases is hard.
+         * So to simplify, a and b are allocated in all conditions.
+         * Also to simplify, in_fd are nonnegative if open, negative
+         * otherwise. */
+        MVM_free(b);
+        MVM_free(a);
+        /* If any of these fail there is nothing
+         * further to do, since we're already failing */
+        if (in_fd >= 0)
+            uv_fs_close(tc->loop, &req, in_fd, NULL);
+        if (out_fd >= 0)
+            uv_fs_close(tc->loop, &req, out_fd, NULL);
+        /* This function only throws adhoc errors, so the message is for
+         * progammer eyes only */
+        MVM_exception_throw_adhoc(tc, "Failed to copy file: %s", error);
+    }
 }
 
+
 /* rename one file to another. */
 void MVM_file_rename(MVMThreadContext *tc, MVMString *src, MVMString *dest) {
     char * const a = MVM_string_utf8_encode_C_string(tc, src);

@p6rt
Copy link
Author

p6rt commented Feb 25, 2015

From @usev6

Looks like this was fixed with commit MoarVM/MoarVM@8352aebb95

The copy already works as expected with a rakudo built on latest MoarVM​:

$ perl6-m --version
This is perl6 version 2015.02-51-g673f50e built on MoarVM version 2015.02-12-ga2dc16d
$ dd if=/dev/zero of=123796.orig bs=1G count=4 2> /dev/null
$ perl6-m -e 'copy("123796.orig".IO, "123796.copy".IO)'
$ ls -l 123796.*
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:33 123796.copy
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:18 123796.orig

@p6rt
Copy link
Author

p6rt commented Mar 1, 2015

From @usev6

On Wed Feb 25 12​:44​:50 2015, bartolin@​gmx.de wrote​:

Looks like this was fixed with commit
MoarVM/MoarVM@8352aebb95

The copy already works as expected with a rakudo built on latest
MoarVM​:

$ perl6-m --version
This is perl6 version 2015.02-51-g673f50e built on MoarVM version
2015.02-12-ga2dc16d
$ dd if=/dev/zero of=123796.orig bs=1G count=4 2> /dev/null
$ perl6-m -e 'copy("123796.orig".IO, "123796.copy".IO)'
$ ls -l 123796.*
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:33 123796.copy
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:18 123796.orig

Since we had a bump for MoarVM and nqp this works now with latest Rakudo. I refrain from adding a test to roast ;-)

I'm closing this ticket as resolved.

1 similar comment
@p6rt
Copy link
Author

p6rt commented Mar 1, 2015

From @usev6

On Wed Feb 25 12​:44​:50 2015, bartolin@​gmx.de wrote​:

Looks like this was fixed with commit
MoarVM/MoarVM@8352aebb95

The copy already works as expected with a rakudo built on latest
MoarVM​:

$ perl6-m --version
This is perl6 version 2015.02-51-g673f50e built on MoarVM version
2015.02-12-ga2dc16d
$ dd if=/dev/zero of=123796.orig bs=1G count=4 2> /dev/null
$ perl6-m -e 'copy("123796.orig".IO, "123796.copy".IO)'
$ ls -l 123796.*
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:33 123796.copy
-rw-r--r-- 1 christian christian 4294967296 Feb 25 21​:18 123796.orig

Since we had a bump for MoarVM and nqp this works now with latest Rakudo. I refrain from adding a test to roast ;-)

I'm closing this ticket as resolved.

@p6rt
Copy link
Author

p6rt commented Mar 1, 2015

@usev6 - Status changed from 'open' to 'resolved'

@p6rt p6rt closed this as completed Mar 1, 2015
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