Return to Answer

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    int numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    ssize_t numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

You might have misinterpreted strncpy:

Copies at most count characters of the character array pointed to by src (including the terminating null character, but not any of the characters that follow the null character)

sun_path is also expected to be null-terminated. The - 1 is not necessary.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    int numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    int numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *.

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

You might have misinterpreted strncpy:

Copies at most count characters of the character array pointed to by src (including the terminating null character, but not any of the characters that follow the null character)

sun_path is also expected to be null-terminated. The - 1 is not necessary.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    int numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string. Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.

Overall you're doing a good job of error checking. Keep that up - I see too much production code that doesn't.

In the signature to fatal, make message a const char *. Similar for get_data(argv).

Consider making a header shared with the server and client, and moving SOCKET_PATH into it.

If those 8 space indentations are caused by hard tabs, use soft (space) indentation instead. If they're actually 8 spaces, reduce them to 4.

You might have misinterpreted strncpy:

Copies at most count characters of the character array pointed to by src (including the terminating null character, but not any of the characters that follow the null character)

sun_path is also expected to be null-terminated. The - 1 is not necessary.

Rather than sizeof on the type like

sizeof(struct sockaddr_un)

prefer sizeof on the variable, in this case socket_addr. It will remain true if the type of the variable changes, and I consider it more visually consistent in most expressions.

The second argument to listen is unclear unless you read the documentation or have a good memory. For long function signatures, I typically comment the parameter name with a parameter on each line; but here that's slightly overkill and the alternative is to declare an int backlog = 8 and pass that in.

This:

                    fatal("Cannot accept connections");
                    sleep(1);

probably intended to use perror instead of fatal.

Whereas a lot of C world unfortunately considers in-predicate mutation like this to be idiomatic:

while ((numread = read(connect_fd, buffer, BUFFER_LEN)) > 0)

it's a pain to read and debug. Instead write

while (1) {
    int numread = read(...);
    if (numread < 1)
        break;
}

printf is not the simplest API contract to use when all you want to do is print a string here: printf("%s", buffer). Instead consider fwrite. This will also remove the need for you to insert null termination.

I dearly hope that you're using at least C99, in which case these two lines:

        FILE* fp;
        fp = fopen(argv[1],"r");

should be combined.