Reputation: 667
I'm trying to write a function to copy a file in C. I need it to work with any type of file whether it's a text, binary file or some other format. This is what I have right now, but it seems like my implementation is broken. Can someone point out what I'm doing wrong and how to fix it?
// Copies the file from source to destination and returns number of bytes written
ssize_t copy_file(char* source, char* destination, int size)
{
if (source == NULL || destination == NULL || access(source, F_OK) == -1)
return 0;
int fd_to = open(destination, O_WRONLY | O_CREAT | O_TRUNC, 0777);
int fd_from = open(source, O_RDONLY);
char* buffer = malloc(sizeof(size));
ssize_t written;
if (fd_to < 0 | fd_from < 0)
return 0;
read(fd_from, buffer, size);
written = write(fd_to, buffer, size);
close(fd_to);
close(fd_from);
free(buffer);
return written;
}
Upvotes: 0
Views: 1378
Reputation: 50110
This isn't why it doesn't work (assuming it's correct) but --
Don't try to read the whole file into memory at once. Allocate a fixed size (1000 bytes say) buffer and loop reading a chunk and writing a chunk till end of file.
Upvotes: 0
Reputation: 57388
Having a buffer as large as the file is not economical for large buffer values ("large" depending on platform and OS, but I doubt it exceeds, say, one megabyte before hitting diminishing returns). On some systems, you could be allowed to allocate much more than the physical available memory, the buffer being backed up by a swap area on disk. At this point if you tried to copy the whole of the file in one fell swoop, you might end up reading and writing most of the file to the swap area, then reading back from the swap area to the new file, effectively doubling (at least) copy times.
So I'd use a loop.
You would also need to check errors in memory allocation and file writing, and consider that size being an int
might cause problems with large files (2 GB is an accessible file size nowadays, yet it will overflow a 32-bit signed integer).
// Copies a part of a file from source to destination
// and returns number of bytes written.
// if input size is < 0, copies the whole file.
ssize_t copy_file(char* source, char* destination, int size)
{
if ((source == NULL) || (destination == NULL) || (access(source, F_OK) == -1)) {
return 0;
}
#define BUFFER_SIZE 1048576
char* buffer = malloc(BUFFER_SIZE);
if (NULL == buffer) {
return 0;
}
int fd_from = open(source, O_RDONLY);
if (fd_from < 0) {
free(buffer);
return 0;
}
int fd_to = open(destination, O_WRONLY | O_CREAT | O_TRUNC, 0777);
if (fd_to < 0) {
free(buffer);
// Avoid leaking a file handle in case of error.
close(fd_from);
return 0;
}
ssize_t written = 0;
// This checks that size is != 0.
// As a result, passing a size < 0 will copy the whole source,
// whatever its length.
// The condition is written explicitly, deliberately (a simple
// while(size) might be overlooked or mistaken for a bug).
while((size > 0)||(size < 0)) {
int ch_r;
ch_r = read(fd_from, buffer, BUFFER_SIZE);
if (ch_r) {
if (ch_r != write(fd_to, buffer, ch_r)) {
// Out of storage space?
close(fd_from);
close(fd_to);
free(buffer);
unlink(destination);
return 0;
}
} else {
// finished
break;
}
written += ch_r;
// We do have a problem of integer size. if
// sizeof(int) is 4 (32bit), files or sizes larger than 2 GB will
// likely misbehave.
size -= ch_r;
}
close(fd_to);
close(fd_from);
free(buffer);
return written;
}
Also, you might find it useful to return the error status instead of the size. If you return zero, then you know that the number of written bytes equals the input size. If you need to return both values, you can put the error in a variable passed by pointer:
ssize_t copy_file(char* source, char* destination, int size, int *status)
{
*status = 0; // Begin with "no error"
...
if (NULL == buffer) {
*status = -8; // -8 stands for "out of memory"
return 0;
}
...
This way, in case of an error, you will know why the routine returned zero. Also you will be able to create zero-length files in case of need (the function will return 0, but status will also be 0, indicating that a write of zero bytes is no error).
To copy a regular file, with no need to specify the file size:
// Copies a file from source to destination
// and returns number of bytes written.
ssize_t copy_file(char* source, char* destination)
{
if ((source == NULL) || (destination == NULL) || (access(source, F_OK) == -1)) {
return 0;
}
#define BUFFER_SIZE 1048576
char* buffer = malloc(BUFFER_SIZE);
if (NULL == buffer) {
return 0;
}
int fd_from = open(source, O_RDONLY);
if (fd_from < 0) {
free(buffer);
return 0;
}
int fd_to = open(destination, O_WRONLY | O_CREAT | O_TRUNC, 0777);
if (fd_to < 0) {
free(buffer);
// Avoid leaking a file handle in case of error.
close(fd_from);
return 0;
}
ssize_t written = 0;
// Infinite loop, exiting when nothing more can be read
for(;;) {
int ch_r;
ch_r = read(fd_from, buffer, BUFFER_SIZE);
if (ch_r) {
if (ch_r != write(fd_to, buffer, ch_r)) {
// Out of storage space?
close(fd_from);
close(fd_to);
free(buffer);
unlink(destination);
return 0;
}
} else {
// finished
break;
}
written += ch_r;
}
close(fd_to);
close(fd_from);
free(buffer);
return written;
}
Upvotes: 2
Reputation: 686
I presume you need to copy a file of arbitrary length, and the size argument passed to the function is the buffer size, as opposed to the file size. You need to do malloc(size)
, not malloc(sizeof(size))
, btw. Most important, you need a loop containing read() and write(), something like
size_t rd_len, wr_len;
do {
rd_len = read(fd_from, buffer, size);
wr_len = write(fd_to, buffer, size);
/* check that wr_len == rd_len */
written += wr_len;
while (wr_len > 0);
Upvotes: 1
Reputation: 37940
sizeof(size)
returns the size of the datatype of the variable size
, which would typically be 4 for an int
- so your buffer always contains 4 bytes. Use malloc(size)
instead. Also, you only read and write one buffer - you need to use a loop to repeat the process if the file is larger than the buffer size.
Also, use ||
instead of |
for logical OR in if (fd_to < 0 | fd_from < 0)
.
Upvotes: 1