[PATCH] core/coio_file: copyfile -- Make it behave as regular cp

[PATCH] core/coio_file: copyfile -- Make it behave as regular cp

[Cyrill Gorcunov]

Traditional cp utility opens destination with O_TRUNC
flag, iow it drops old content of the target file if
such exists.

Fixes #4181
---
https://github.com/tarantool/tarantool/issues/4181

Guys, take a look please, this is change in behaviour but
should be acceptable.

Also I just wondered that most of tests in test/app/fio.test.lua
do pass octal modes as 0777 while lua's number parser simply doesn't
understand octal form. So that I had to pass mode 420 by hands which
is an alias to 0644.

 src/lib/core/coio_file.c |  2 +-
 test/app/fio.result      | 23 +++++++++++++++++++++++
 test/app/fio.test.lua    |  9 ++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 3caf185a5..3359f42bc 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -584,7 +584,7 @@ coio_do_copyfile(eio_req *req)
 		goto error;
 	}
 
-	int dest_fd = open(eio->copyfile.dest, O_WRONLY | O_CREAT,
+	int dest_fd = open(eio->copyfile.dest, O_WRONLY|O_CREAT|O_TRUNC,
 			   st.st_mode & 0777);
 	if (dest_fd < 0) {
 		goto error_dest;
diff --git a/test/app/fio.result b/test/app/fio.result
index 879e0a767..32f8d6868 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -721,6 +721,9 @@ file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
 ---
 ...
+file5 = fio.pathjoin(tree, 'file.5')
+---
+...
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
 ---
 ...
@@ -775,6 +778,26 @@ errinj.set('ERRINJ_COIO_SENDFILE_CHUNK', -1)
 ---
 - ok
 ...
+--- test the destination file is truncated
+fh5 = fio.open(file5, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+---
+...
+fh5:write("to be truncated")
+---
+- true
+...
+fh5:close()
+---
+- true
+...
+fio.copyfile(file4, file5)
+---
+- true
+...
+fio.stat(file4, file5) ~= nil
+---
+- true
+...
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 ---
 ...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 1255b2804..b0d6c5f49 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -38,7 +38,6 @@ file2 = fio.pathjoin(tmpdir, 'file.2')
 file3 = fio.pathjoin(tmpdir, 'file.3')
 file4 = fio.pathjoin(tmpdir, 'file.4')
 
-
 st, err = pcall(fio.open, nil)
 st
 err:match("open") ~= nil
@@ -231,6 +230,7 @@ file1 = fio.pathjoin(tmp1, 'file.1')
 file2 = fio.pathjoin(tmp2, 'file.2')
 file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
+file5 = fio.pathjoin(tree, 'file.5')
 
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
 fh1:write("gogo")
@@ -249,6 +249,13 @@ fio.copyfile(file1, file4)
 fio.stat(file1, file4) ~= nil
 errinj.set('ERRINJ_COIO_SENDFILE_CHUNK', -1)
 
+--- test the destination file is truncated
+fh5 = fio.open(file5, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+fh5:write("to be truncated")
+fh5:close()
+fio.copyfile(file4, file5)
+fio.stat(file4, file5) ~= nil
+
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 res
 err:match("failed to copy") ~= nil
-- 
2.20.1

Re: [PATCH] core/coio_file: copyfile -- Make it behave as regular cp

[Vladimir Davydov]

On Sat, May 04, 2019 at 06:49:22PM +0300, Cyrill Gorcunov wrote:
> Traditional cp utility opens destination with O_TRUNC
> flag, iow it drops old content of the target file if
> such exists.
> 
> Fixes #4181
> ---
> https://github.com/tarantool/tarantool/issues/4181
> 
> Guys, take a look please, this is change in behaviour but
> should be acceptable.
> 
> Also I just wondered that most of tests in test/app/fio.test.lua
> do pass octal modes as 0777 while lua's number parser simply doesn't
> understand octal form. So that I had to pass mode 420 by hands which
> is an alias to 0644.

I guess we should change 0644 to tonumber('644', 8) across the whole
test suite. Could you please do it in a separate patch?

Anyway, the new test case passes both with and without this patch, which
means it's useless. Please make sure it fails without the changes made
to the code by this patch.

Thanks.

Re: [PATCH] core/coio_file: copyfile -- Make it behave as regular cp

[Cyrill Gorcunov]

On Mon, May 06, 2019 at 11:37:29AM +0300, Vladimir Davydov wrote:
> > 
> > Also I just wondered that most of tests in test/app/fio.test.lua
> > do pass octal modes as 0777 while lua's number parser simply doesn't
> > understand octal form. So that I had to pass mode 420 by hands which
> > is an alias to 0644.
> 
> I guess we should change 0644 to tonumber('644', 8) across the whole
> test suite. Could you please do it in a separate patch?

Will do.

> Anyway, the new test case passes both with and without this patch, which
> means it's useless. Please make sure it fails without the changes made
> to the code by this patch.

OK

[PATCH v2] core/coio_file: copyfile -- Make it behave as regular cp

[Cyrill Gorcunov]

Traditional cp utility opens destination with O_TRUNC
flag, iow it drops old content of the target file if
such exists.

Fixes #4181
---
 https://github.com/tarantool/tarantool/issues/4181

 v2: Update the test to compare file contents

 src/lib/core/coio_file.c |  2 +-
 test/app/fio.result      | 41 ++++++++++++++++++++++++++++++++++++++++
 test/app/fio.test.lua    | 13 +++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/src/lib/core/coio_file.c b/src/lib/core/coio_file.c
index 3caf185a5..3359f42bc 100644
--- a/src/lib/core/coio_file.c
+++ b/src/lib/core/coio_file.c
@@ -584,7 +584,7 @@ coio_do_copyfile(eio_req *req)
 		goto error;
 	}
 
-	int dest_fd = open(eio->copyfile.dest, O_WRONLY | O_CREAT,
+	int dest_fd = open(eio->copyfile.dest, O_WRONLY|O_CREAT|O_TRUNC,
 			   st.st_mode & 0777);
 	if (dest_fd < 0) {
 		goto error_dest;
diff --git a/test/app/fio.result b/test/app/fio.result
index 879e0a767..39b2ad0b5 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -721,6 +721,12 @@ file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
 ---
 ...
+file5 = fio.pathjoin(tree, 'file.5')
+---
+...
+file6 = fio.pathjoin(tree, 'file.6')
+---
+...
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
 ---
 ...
@@ -775,6 +781,41 @@ errinj.set('ERRINJ_COIO_SENDFILE_CHUNK', -1)
 ---
 - ok
 ...
+--- test the destination file is truncated
+fh5 = fio.open(file5, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+---
+...
+fh5:write("template data")
+---
+- true
+...
+fh5:close()
+---
+- true
+...
+fh6 = fio.open(file6, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+---
+...
+fh6:write("to be truncated")
+---
+- true
+...
+fio.copyfile(file5, file6)
+---
+- true
+...
+fh6:seek(0)
+---
+- 0
+...
+fh6:read()
+---
+- template data
+...
+fh6:close()
+---
+- true
+...
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 ---
 ...
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 1255b2804..ffb15c057 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -231,6 +231,8 @@ file1 = fio.pathjoin(tmp1, 'file.1')
 file2 = fio.pathjoin(tmp2, 'file.2')
 file3 = fio.pathjoin(tree, 'file.3')
 file4 = fio.pathjoin(tree, 'file.4')
+file5 = fio.pathjoin(tree, 'file.5')
+file6 = fio.pathjoin(tree, 'file.6')
 
 fh1 = fio.open(file1, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 0777)
 fh1:write("gogo")
@@ -249,6 +251,17 @@ fio.copyfile(file1, file4)
 fio.stat(file1, file4) ~= nil
 errinj.set('ERRINJ_COIO_SENDFILE_CHUNK', -1)
 
+--- test the destination file is truncated
+fh5 = fio.open(file5, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+fh5:write("template data")
+fh5:close()
+fh6 = fio.open(file6, { 'O_RDWR', 'O_TRUNC', 'O_CREAT' }, 420)
+fh6:write("to be truncated")
+fio.copyfile(file5, file6)
+fh6:seek(0)
+fh6:read()
+fh6:close()
+
 res, err = fio.copyfile(fio.pathjoin(tmp1, 'not_exists.txt'), tmp1)
 res
 err:match("failed to copy") ~= nil
-- 
2.20.1

Re: [PATCH v2] core/coio_file: copyfile -- Make it behave as regular cp

[Vladimir Davydov]

Pushed to master, 2.1, 1.10, thanks!