Merging two files into a third in C (fread and fwrite)

Question

I'm trying to make a function that takes a series of files and writes the contents of each one into a new one.

#include <stdio.h>
#include <stdlib.h>

    void file_copy(FILE *f1, FILE *f2) {
    char buffer[BUFFER_SIZE];
    size_t sz;
    sz = fread(buffer, sizeof(buffer),1,f1);
    while (sz == 1) {
        fwrite(buffer, sz,1,f2);
        sz = fread(buffer, sizeof(buffer),1,f1);
    }
}

int main(int argc, char const *argv[])
{
    FILE *f_in, *f_out;
    int i;
    if (argc < 3)
    {
        puts("Not enough arguments.");
        exit(1);
    }

    if ((f_out = fopen(argv[argc-1], "w")) == NULL)
    {
        printf("Can't open %s for writing.\n", argv[argc-1]);
        exit(1);
    }

    for (i = 0; i < argc-1; ++i)
    {
        if ((f_in = fopen(*++argv, "r")) == NULL)
        {
            printf("Can't open %s for reading\n", *argv);
            exit(1);
        }
        else
            file_copy(f_in, f_out);
        close(f_in);
        close(f_out);
    }


    return 0;
}

and I dont' get any output in my output file. It seems to be closing them just right.

[Why is “while ( !feof (file) )” always wrong?](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong) — kaylum, Jun 30 '16 at 22:06
I think you are misinformed of what `fread()` returns. It returns the number of items read. In your statement, the size is 1024, so expect a return of `1` meaning 1024 bytes read or `0` less than 1024 bytes read. If you had swapped the size and count, then the value returned by `fread()` will be aligned to your expectation. Meaning if you try to read 1024 items of size 1 byte, then the value will return how may bytes was read. — alvits, Jun 30 '16 at 22:25
You fixed the function but everything else are still broken. — alvits, Jun 30 '16 at 22:41
when opening a file with `fopen()` it needs to be closed with `fclose()`, NOT `close()`, and `close()` requires the statement: `#include ` — user3629249, Jul 02 '16 at 15:01
the posted code is using a macro: `BUFFER_SIZE` but that macro is not defined anywhere in the posted code. — user3629249, Jul 02 '16 at 15:02
when compiling, always enable all the warnings, then fix those warnings. (for `gcc`, at a minimum use: `-Wall -Wextra -pedantic` I also use: `-Wconversion -std=gnu99` — user3629249, Jul 02 '16 at 15:04
for ease of readability and understanding: 1) please indent the code consistently. Indent after every opening brace '{'.. Un-indent before every closing brace '}'. Suggest using 4 spaces for each indent level (never use tabs for indenting) as that is wide enough to be visible even with variable width fonts. 2) separate code blocks (for, if, else, while, do...while, switch, case, default) via a blank line. 3) variable names should indicate `usage` or `content` (or better, both) — user3629249, Jul 02 '16 at 15:08
for ease of readability and documentation of the code, follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* — user3629249, Jul 02 '16 at 15:10

krzydyn · Answer 1 · 2016-06-30T22:19:07.237

1

There are Several fails:

buffer must be allocated somehow
write have to write number of bytes read, not full buffer

So it should be like this:

#define BUF_SIZE 1024

void file_copy(FILE *f1, FILE *f2) {
    char buffer[BUF_SIZE];

    while (!feof(f1)) {
        size_t r = fread(buffer, 1, sizeof(buffer), f1);
        fwrite(buffer, 1, r, f2);
    }
}

edited Jun 30 '16 at 22:19

answered Jun 30 '16 at 21:56

krzydyn

1,012
9
19

I tried that and now it doesn't write anything on the file. – Telefang Jun 30 '16 at 22:01
Probably you have other mistake outside this function. Maybe not closing the file. – krzydyn Jun 30 '16 at 22:03
I posted my full code there ad I don't think that is the problem. – Telefang Jun 30 '16 at 22:05
For sure you have to move `close(f_out)` out of the `for` loop. – krzydyn Jun 30 '16 at 22:09
1

@Telefang - You are both wrong. Second parameter of `fread()` or `fwrite()` tells the size of the buffer while the third parameter tells how many of these size to read or write. If your file is smaller than 1024 bytes, the value of `r` will be 0. If the file is greater than 1024 bytes, the value of `r` each read will be 1. In short you are reading 1 count of 1024 bytes per call. 1 is returned when it successfully read 1024 bytes otherwise ii is 0 when shorter than 1024 bytes is read. – alvits Jun 30 '16 at 22:17
I made the corrections in my code and still nothing in my ouput file. – Telefang Jun 30 '16 at 22:24
Program `args` counts from 1 to `argc`, and `argv[0]` is the binary name – krzydyn Jun 30 '16 at 22:35

score 1 · Accepted Answer · answered Jun 30 '16 at 22:32

Did you plan to save the text from the list of files in the last file? It isn't really clear. There are multiple problems with the code aside from unreadable syntax. You should be using fclose instead of close. And reading a number of one byte elements instead of one element of a giant array.

Here is the fixed code:

#include <stdio.h>
#include <stdlib.h>

void file_copy(FILE *f1, FILE *f2) {
    char buffer[1024];
    ssize_t sz;

    while (!feof(f1)) {
        sz = fread(buffer, 1, sizeof(buffer), f1);

        fwrite(buffer, 1, sz, f2);
    }
}

int main(int argc, char const *argv[])
{
    FILE *f_in, *f_out;
    int i;
    if (argc < 3)
    {
        puts("Not enough arguments.");
        exit(1);
    }

    if ((f_out = fopen(argv[argc-1], "w")) == NULL)
    {
        printf("Can't open %s for writing.\n", argv[argc-1]);
        exit(1);
    }

    for (i = 1; i < argc-1; i++)
    {
        if ((f_in = fopen(argv[i], "r")) == NULL)
        {
            printf("Can't open %s for reading\n", *argv);
            exit(1);
        }
        else
        {
            file_copy(f_in, f_out);
            fclose(f_in);
        }
    }
    fclose(f_out);

    return 0;
}

`argc` is the count. `argv` in an array taking index. So the last element will be `argv[argc-1]`. https://stackoverflow.com/questions/3024197/what-does-int-argc-char-argv-mean — ilya1725, Jun 30 '16 at 23:00

Aleins · Answer 3 · 2016-06-30T22:40:24.730

0

You should definitely know how much to write, because you end up writing 1024 bytes no matter what, as is now.

Depending on what file type you are dealing with, this is an option for txt-files at least:

void file_copy(FILE *f1, FILE *f2) {

    fseek(f1, 0, SEEK_END);
    int buf_size = ftell(f1);
    char buf[buf_size];

    //Reset file position
    fseek(f1, 0, SEEK_SET);
    sz = fread(buf, sizeof(buf), 1, f1);
    fwrite(buf, sz, 1, f2);
}

edited Jun 30 '16 at 22:40

answered Jun 30 '16 at 22:34

Aleins

43
8

This would blow when you try to copy DVD iso file. It is never a good idea to buffer the whole file. The while loop will only execute once because the whole file is read into buffer. Why even use a loop? – alvits Jun 30 '16 at 22:38
Discovered that now, clarified type of file, will remove the loop, my mistake @alvits – Aleins Jun 30 '16 at 22:41

Merging two files into a third in C (fread and fwrite)

3 Answers3