Implementation of cp program that prints the number of bytes copied

Question

I have written the code for implementation of the cp program that prints the number of bytes copied when the user presses Ctrl-C. Could someone please review this code and provide feedback. Thanks a lot.

#include<stdio.h>
#include<stdlib.h>
#include<unistd.h>
#include<fcntl.h>
#include<error.h>
#include<signal.h>
#define buffer_size 512

void signal_handler(int num);
void write_buf(int des_fd, char *buffer, size_t rlen, int *bytes);
int flag = 0;

void signal_handler(int num)
{
    printf("The caught signal number is : %d\n", num);
    flag = 1;
}

void write_buf(int des_fd, char *buffer, size_t rlen, int *bytes)
{
    int wlen;

    while (1) {
        wlen = write(des_fd, buffer, rlen);
        if (wlen == -1)
            error(1, 0, "error in writing the file\n");
        *bytes = *bytes + wlen;
        buffer = buffer + wlen;
        rlen = rlen - wlen;
        if (rlen == 0)
            break;
    }
}

int main()
{
    int src_fd;
    int des_fd;
    int rlen;
    int bytes;
    char buffer[buffer_size];

    signal(SIGINT, signal_handler);
    src_fd = open("src_text", O_RDONLY);
    if (src_fd == -1)
        error(1, 0, "error in opening the source-file\n");
    des_fd = open("des_txt", O_WRONLY);
    while (1) {
        rlen = read(src_fd, buffer, sizeof(buffer));
        if (rlen == -1)
            error(1, 0, "error in reading the file\n");
        if (rlen == 0)
            break;
        write_buf(des_fd, buffer, rlen, &bytes);
        if (flag == 1)
            printf("The number of bytes copied is :%d\n", bytes);
        flag = 0;
    }
}
```

Answer 1

• Do not call printf from the signal handler. It is not signal safe. man sigaction for details.

• write_buf is somewhat convoluted. while (rlen > 0) seems more straightforward.

  Also, it is forced to write the entire buffer. It means that your program only pays attention to the Ctrl-C in between of writes. You may get more crispy resolution by testing wlen < rlen and inspecting errno; if write was interrupted, it would be EINTR.

  void function with an in-out parameter (bytes in your case) is strange to say the least. The void function is not supposed to return anything, and in-out parameters are generally unclean. Just return bytes.

• error() is a GNU extension and should not be used in programs intended to be portable. In any case, the pass errno as a second parameter. The user is very interested what exactly was wrong.

• If the program was not interrupted, it doesn't print number of bytes copied. Looks like a bug (or a very dubious design decision) to me.

Answer 2

Avoid unnecessary forward declarations

The forward declarations of signal_handler() and write_buf() are unnecessary and can be removed. Try to avoid repeating code, the less chance there is for errors.

Make variables and functions static where appropriate

If variables and functions do not need to be used from other source files, you can mark them static. This will allow the compiler to better optimize the code.

Make flag an atomic variable

It is generally not safe to just set flag = 1 in the signal handler and expect the main() function to see it change. That is because the compiler might in some circumstances be allowed to assume that nothing could change the variable flag between iterations in the while-loop, and thus optimize away the check. In this case it will work because the compiler cannot assume that the I/O functions you call in the loop won't change global variables. However, if you were not calling any library functions in the loop, the compiler would probably have elided the check.

The quick way to fix this kind of issue is to make flag volatile. If you can use C11 or later, then you might want to use atomic operations for this.

Use unsigned long long or uint64_t to store the total size read

An int typically only holds values up to 2 billion. Files can easily be larger than 2 gigabytes, so you want to use a type that can hold larger values. An unsigned long long will likely be 64 bits, but it is even better to be explicit and use uint64_t from stdint.h.

Avoid changing the behavior of common signals

The expectation is normally that SIGINT causes a program to exit immediately, not to print some information and continue. However, it is nice to handle SIGINT in your application and have it exit gracefully, in your case by exitting the loop, then printing the amount of bytes copied:

while (flag) {
    ...
}

printf("The number of bytes copied is: %llu\n", bytes);

Alternatives to signals

Using UNIX signals is always a bit tricky, since there are only a few things you can do in a signal handler that are safe. If you want to provide a progress indicator while copying something, and want the progress indicator to be updated once every second, then a possible way is to create a thread to do this. One issue is that creating a thread is easy, shutting it down properly is another matter.

You can also try to use a buffer size that is so large that it takes about a second to read and write, so you can just print the progress indicator once every iteration in the loop. Or, if you want to keep the buffer small (or if you have another problem where each loop iteration is much faster than a second), you can just have a simple counter that increments for each iteration, and only update the progress indicator once so many iterations.