1
\$\begingroup\$

For learning purposes I'm writing a very simple mqueue based file transfer server. Below is a thread function that takes care of an incoming request:

static void * handle_client_request(const char *request_string) {
    char request_pathname[_part_delim_alloc_size],
         request_fifo_name[_part_delim_alloc_size];

    bool parse_ret = parse_mqueue_request_string(request_string, (char *[]) {request_pathname, request_fifo_name});
    if(!parse_ret) {
        fprintf(stderr, PROGNAME ": error parsing mqueue request string: %s\n", request_string);
        free(request_string);
        return NULL;
    }

    fprintf(stdout, PROGNAME ": received new request: (%s, %s) -- handling\n", request_pathname, request_fifo_name);

    // open requested file for reading
    int file_fd = open(request_pathname, O_RDONLY);
    if(file_fd == -1) {
        perror(PROGNAME ": error opening requsted file");
        free(request_string);
        return NULL;
    }

    // construct full FIFO path
    char full_fifo_path[strlen(FIFO_STORAGE_PATH) + strlen(request_fifo_name) + 1];
    strcpy(full_fifo_path, FIFO_STORAGE_PATH);
    strncat(full_fifo_path, request_fifo_name, 11);

    // open FIFO for writing only -- this may block until client also opens the FIFO
    int fifo_fd = open(full_fifo_path, O_WRONLY);
    if(fifo_fd == -1) {
        perror(PROGNAME ": failed opening FIFO for writing");
        close(file_fd);
        free(request_string);
        return NULL;
    }

    void *buf = malloc(DEFAULT_BUFSIZE); // 128 KiB, 2^17 bytes
    if(!buf) {
        perror(PROGNAME ": failed allocating buffer");
        close(file_fd);
        close(fifo_fd);
        free(request_string);
        return NULL;
    }

    size_t bread;
    while((bread = read(file_fd, buf, DEFAULT_BUFSIZE)) > 0) {
        // read call complete, "bread" bytes are now in "buf"
        ssize_t bwritten = write(fifo_fd, buf, bread);

        if(bwritten == -1) {
            // failed writing the batch of bytes received
            fprintf(stderr, PROGNAME ": failed writing to FIFO - request aborted (%s): %s\n", full_fifo_path, strerror(errno));
            free(buf);
            free(request_string);
            close(file_fd);
            close(fifo_fd);
            return NULL;
        }

        /* fprintf(stdout, PROGNAME ": read %lu bytes of data and wrote %ld bytes of data\n", bread, bwritten); */
    }

    fprintf(stdout, PROGNAME ": done processing request (%s, %s) last bread: %lu\n", request_pathname, request_fifo_name, bread);

    close(file_fd);
    close(fifo_fd);
    free(buf);

    return NULL;
}

Its purpose is to transfer a local file (that the client requests) over a FIFO that the client connects to.

The size of the buffer for read(2) and write(2) I've decided on is 128KiB. Increasing it doesn't seem to have an positive effect on performance.

I am wondering - are there any trivial (or not so trivial) optimizations I could apply to this function to gain an ever so slight increase in performance?

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Const

I consider

const char *request_string

and

free(request_string);

to be in conflict. If the contract for this function is that it is responsible for freeing the string it was given, it should not be const. Conversely, if you really think it should be const, shift responsibility of freeing it onto the caller.

Stack allocation

In C terms, support for dynamic stack allocation of the kind seen in

char full_fifo_path[strlen(FIFO_STORAGE_PATH) + strlen(request_fifo_name) + 1];

is relatively new. If I were you I would be concerned that a hostile caller could pass a huge string in request_string, which parses into a huge request_fifo_name and blows the stack. As long as you take care to free the memory yourself through the various return paths in this function, I would consider malloc safer.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ In this particular case the maximum allowed length of request_string is set as a runtime parameter, although it cannot exceed linux's maximum mqueue message length (8192 bytes), which is still far less than the stack size. Still a good idea to switch to malloc though, will definitely do that! Thank you! \$\endgroup\$
    – bool3max
    Commented May 25, 2020 at 21:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.