1
\$\begingroup\$

These functions will open a relative path, read or write to the file, then close. The purpose of these functions is to avoid duplicate code patters for accessing different files that only require one read/write prior to closing. Concerns:

  • Should the functions log errors? Or conventionally is this a job for the caller?
  • Should Read or Write or both retry until either an error occurs or all bytes are written? It seems either this will result in inconsistency or duplicate code. When would the extra complexity be worth the more graceful error handling?
  • Which system calls (if any) should I retry if errno == EINTR? I feel this is a slippery slope, if I do it here, I will feel obliged to do it with every system call for consistency, increasing complexity and requiring modifying older code.
  • How can I avoid duplicate code if the logic for the functions is almost identical except for one calling read and the other calling write?

Below is a crude but 'working' starting point

static bool Read(int File, const char *const restrict Name, void *const restrict Ptr, const size_t Size) {
    if ((File = openat(File, Name, O_RDONLY)) == -1)
        E: return perror(Name), 1;
    const bool R = pread(File, Ptr, Size, 0) == -1;
    if (R)
        perror(Name), (void)memset(Ptr, 0, Size);
    if (close(File))
        goto E;
    return R;
}
static bool Write(int File, const char *const restrict Str, const void *restrict Ptr, size_t Size) {
    if ((File = openat(File, Str, O_WRONLY|O_CREAT|O_TRUNC)) == -1)
        E: return perror(Str), 1;
    for (ssize_t R;; Ptr += R) {
        if ((R = write(File, Ptr, Size)) == -1)
            perror(Str);
        else if (Size -= R)
            continue;
        break;
    }
    if (close(File))
        goto E;
    return (bool)Size;
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ What's your design goal for these wrappers? When calling Read(), how should the caller know how large a buffer to supply? \$\endgroup\$ Feb 27 at 5:34
  • \$\begingroup\$ @200_success The design goal is to reduce the repeated open read/write close pattern in projects, they are used in various places, where the buffer size is known by the caller and passed to the function. \$\endgroup\$
    – user255905
    Feb 27 at 5:48
  • \$\begingroup\$ For the reference I am creating a function to abstract away the process of retrying read/write until either completion or error, then calling it from within my revised Read and Write functions \$\endgroup\$
    – user255905
    Feb 28 at 21:12

1 Answer 1

3
\$\begingroup\$

Answers to your questions

Should the functions log errors? Or conventionally is this a job for the caller?

It really depends on whether you want these functions to be part of a generic library, or if they are just private convenience functions. In the former case, I would leave the logging to the caller, and ensure an error code is returned (or possibly errno could be used as you suggested yourself). If the latter, then always logging an error might be exactly what you want, and then leaving it in these functions will make the rest of your code simpler.

Should Read() or Write() or both retry until either an error occurs or all bytes are written? It seems either this will result in inconsistency or duplicate code. When would the extra complexity be worth the more graceful error handling?

You should never assume that all bytes will be read or written in one go, so if the idea of these functions is that they read/write a whole file in one go, then you must handle this.

Which system calls (if any) should I retry if errno == EINTR? I feel this is a slippery slope, if I do it here, I will feel obliged to do it with every system call for consistency, increasing complexity and requiring modifying older code.

That's true. Again, I would say it depends. If you don't plan on using signals in any way in your code, you can probably treat EINTR as an error. If you want to have code that works in all scenarios and want it to handle any recoverable error, then you should handle the possibility of EINTR for all function calls, however tedious.

How can I avoid duplicate code if the logic for the functions is almost identical except for one calling read and the other calling write?

You could write a generic function that takes parameters that tell it whether to read or write, and then implement Read() and Write() as two small wrapper functions that call the generic function. However, consider that Read() takes a non-const pointer to the buffer to read the file into, but Write() takes a const pointer. If you want to make a generic function, you can't just have one Ptr parameter without requiring const-casting, which isn't great.

Code style

The code style you are using is very atypical. Normally, one would not start variable and function names with capitals. It is also very dense, using assignments inside if-statements, and using the comma operator seemingly just to avoid a few braces. goto statements are frowned upon, and while there is some precedence to use them for handling error cases, having a goto jump back into the middle of an if-statement is a terrible practice.

Also avoid using one-letter variable names, unless it is something that is idiomatic, like i for a loop counter, or x/y/z for coordinates.

Don't reuse variables for different purposes

In both functions, File is an input parameter that holds the filedescriptor of a directory (not file!). However, you then reuse it to hold the filedescriptor of the file you opened. This is confusing for someone that reads your code, and might result in problems if someone wants to add another openat() call in the same function.

Just use a separate variable to hold the filedescriptor of the opened file. Also note that compilers will typically do lifetime analysis of variables, and will be able to spot that they can use the same register for both variables.

Inconsistencies

Why are you using pread() inside Read() but the regular write() inside Write()? If you put write() in a loop to handle the case where it doesn't write everything in one go, why not do the same for read() or pread(), which have the exact same issue?

Check errno for recoverable errors

A call to read() or write() might return -1 in case of an error, but not all errors are equal. In particular, -1 can be returned in case nothing was wrong with the file, but the system call was interrupted somehow. In that case, EINTR is returned, and you should continue trying to read/write in that case instead of returning an error.

Return value

A bool return type suggests that the return value indicates success or failure, and usually true means success. However, you return 1 or true when there is an error. I strongly recommend you return false in case of errors, and also avoid returning 1 or sizes cast to bool.

Pointer arithmetic

Pointer arithmetic on void pointers is illegal in C. You should first create a new char * variable and assign the value of the void pointer to it, then perform arithmetic on the new variable.

Does the caller really know the size of a file up front?

Your Read() function tries to read exactly Size bytes from the file, regardless of the actual size of the file. It is quite rare that you know in advance what the size of a file is. The caller could of course first do an fstatat() call, but then you have a possible TOCTTOU issue. If the actual use case is just: "please read this whole file into a buffer", then it would be better to have Read() take care of getting the file size by using fstat() after opening the file, and then allocating a large enough buffer, reading the file into it, and returning the buffer and its size to the caller.

Possible rewrite

Here is how I would rewrite Write():

bool write_buffer(int dirfd, const char *const restrict pathname, const void *restrict buf, size_t count) {
    int fd = openat(dirfd, pathname, O_WRONLY | O_CREAT | O_TRUNC);

    if (fd == -1) {
        warn("Could not open %s", pathname);
        return false;
    }

    unsigned char *ptr = buf;

    while (count) {
        ssize_t result = write(fd, ptr, count);

        if (result == -1) {
            if (errno == EINTR) {
                continue;
            } else {
                warn("Error writing from %s", pathname);
                close(fd);
                return false;
            }
        }

        ptr += result;
        count -= result;
    }

    if (close(fd) == -1) {
        warn("Error closing %s", pathname);
        return false;
    }

    return true;
}

I used warn() here as you can put a bit more information in the error message that way, and differentiate between the error happening during opening, writing or closing the file.

The names of the function parameters were chosen to be the same as those for openat() and write(), so it is more familiar for those who already know those POSIX functions.

And yes, the code is a lot longer, but a lot is just whitespace, and code length is not directly related to binary size or performance. It is much more important that it is readable, correct and maintainable.

There is code duplication in the error handling, but every case is slightly different here; you can't close() if openat() failed, you have to print the error message before calling close() if write() failed, and again you probably can't close() again if close() failed (unless errno == EINTR again, which I didn't handle in my example).

\$\endgroup\$
9
  • \$\begingroup\$ Can open() or close() fail with EINTR? Should I start retrying every system call that fails with EINTR? \$\endgroup\$
    – user255905
    Feb 27 at 15:17
  • \$\begingroup\$ Yes, it turns out even open() can fail (it's documented in the manpages). Whether you should handle it depends: if you know your application doesn't use signals, and the few signals it might get (SIGINT from ctrl-C) would terminate it anyway, then don't bother. But if you are writing these functions as part of a utility library, and want to be able to use those functions in any scenario, then you have to start thinking about it. Also, treating it as an error during open is perhaps OK, but if you do the same during writes, then consider that a partially written file might be bad. \$\endgroup\$
    – G. Sliepen
    Feb 27 at 17:09
  • \$\begingroup\$ Should I treat read and write any differently? Also should I log errors or is that the callers job conventionally? \$\endgroup\$
    – user255905
    Feb 27 at 23:54
  • \$\begingroup\$ I would treat read and write the same if possible. It depends on the rest of the program whether it would be better for these functions to log errors or if it is better left up to the caller. However, if you do want the caller to handle it, you should at least return an error code instead of just a bool. This then also has the advantage that the caller can distinguish, for example, between file not found or insufficient permissions to open the file. \$\endgroup\$
    – G. Sliepen
    Feb 28 at 8:02
  • \$\begingroup\$ I could set errno \$\endgroup\$
    – user255905
    Feb 28 at 15:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy