Fetching filesystem directory listings on a server

Question

I developed a server which takes a directory name then lists files in it and sends this list to a client. And I want that server to work with Telnet.

It works but I have some questions on how I can do this better:

1. My client can take some directory names from arguments and sends them to the server in a cycle. Server responds filenames line by line. After the last line client have to send the next directory. So, how can I split the blocks of lines for each directory? Now I use '/' as a delimiter because this symbol can't be in a file name.

   Example:

   client: 
       dirname1  
   server:
       filename1
       filename2
       /
   client: 
       dirname2
   server:
       filename3
       filename4
       /
   etc ...
   

   Is this a good way?

2. Telnet sends each line with CRLF but my client doesn't. I check each string from the client via the function strchr(buffer,'\r'). If a string has the '\r' symbol, I delete the two last symbols. I think it's strange way, but what do you think?

3. Maybe you have comments about my code style.

This is server.c file:

#include <stdio.h>
#include <sys/types.h> 
#include <sys/socket.h>
#include <netinet/in.h>
#include <dirent.h>

#define BUFFER_SIZE 1024

void doaction(int sock);

void 
error(const char* err)
{
    perror(err);
    exit(1);
}

int 
main(int argc, char  *argv[])
{
    int sockfd, newsockfd, port, pid, clilen;
    struct sockaddr_in server, client;
    int reuse = 1;

    if(argc != 2) {
        printf("usage: server port\n");
        exit(1);
    }

    if ( (sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0)
        error("socket");

    bzero((char *) &server, sizeof(server));
    port = atoi(argv[1]);
    server.sin_family = AF_INET;
    server.sin_addr.s_addr = INADDR_ANY;
    server.sin_port = htons(port);

    if (bind (sockfd, (struct sockadd *) &server, sizeof(server)) < 0)
        error("bind");
    if (setsockopt (sockfd, SOL_SOCKET, SO_REUSEADDR, &reuse,
        sizeof(reuse)) < 0)
        error("setsockopt");

    listen(sockfd, 5);
    clilen = sizeof(client);
    while (1) {
        newsockfd = accept(sockfd, 
                    (struct sockadd *) &client, &clilen);
        if (newsockfd < 0)
            error("accept");
            pid = fork();
            if (pid < 0 )
                error("fork");
            if (pid == 0) {
                close(sockfd);
                doaction(newsockfd);
                exit(0);
            }
            else close(newsockfd);
    }

    return 0;
}

void
doaction(int sock)
{
    ssize_t n;
    char buffer[BUFFER_SIZE] = {0};
    DIR *dir;
    struct dirent *dp;
    int telnet = 0;
    char * pch;

    while ( (n = read(sock, buffer, BUFFER_SIZE)) > 0)
    {       
        pch=strchr(buffer,'\r');
        if (pch != NULL)
        {
            telnet = 1;
            buffer[n-2] = 0;
        }

        if ((dir = opendir (buffer)) == NULL) {
            char str1[] = "\tCan't open the ";
            write(sock , str1 , strlen(str1));
            write(sock , buffer , strlen(buffer));
            write(sock , "\n" , 1);
            if (telnet == 0)
                write(sock , "/" , 1);
            bzero(&buffer, BUFFER_SIZE);
            continue;
        }

        while ((dp = readdir(dir)) != NULL) {
            write(sock , "\t" , 1);
            write(sock , dp->d_name , strlen(dp->d_name));
            write(sock , "\n" , 1);
        }
        if (telnet == 0)
            write(sock , "/" , 1);

        bzero(&buffer, BUFFER_SIZE);

        if (closedir(dir) < 0)
            error("closedir");
    }
    if (n == 0) {
        printf("Client disconnected\n");
    } else if (n < 0) {
        error("recv");
    }
}

And client.c file:

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h> 
#include <sys/types.h> 
#include <sys/socket.h>
#include <arpa/inet.h>

#define BUFFER_SIZE 1024

void 
usage()
{
    printf("usage: client -h host -p port dir1 .... dirN\n");
    exit(1);
}

void 
error(const char* err)
{
    perror(err);
    exit(1);
}

int 
main(int argc, char  *argv[])
{
    int opt, index, n, sockfd;
    struct sockaddr_in server;
    char *host = NULL;
    char * pch;
    int port = -1;
    char buffer[BUFFER_SIZE] = {0};

    while((opt = getopt(argc,argv,"h:p:")) != -1) {
        switch (opt) {
            case 'h':
                host = optarg;
                break;
            case 'p':
                port = atoi(optarg);
                break;
            default:
                usage();            
        }
    }
    if (argc < 1)
        usage();

    if (host == NULL || port == -1)
        usage();    

    bzero((char *) &server, sizeof(server));

    server.sin_family = AF_INET;
    server.sin_addr.s_addr = inet_addr(host);;
    server.sin_port = htons(port);

    if ( (sockfd = socket(AF_INET, SOCK_STREAM, 0)) < 0)
        error("socket");

    if (connect(sockfd, (struct sockadd *) &server, sizeof(server)) < 0)
        error("connect");

    printf("host: %s port: %d - connected\n", host, port);

    for (index = optind; index < argc; index++) {
        printf("%s\n", argv[index]);

        if (write(sockfd , argv[index] , strlen(argv[index])) < 0)
            error("write");

        while (1) {
            bzero(&buffer, BUFFER_SIZE);
            if ( (n = read(sockfd, buffer, BUFFER_SIZE)) > 0)
            {
                pch=strchr(buffer,'/');
                if (pch != NULL)
                {
                    *pch = '\0';
                    printf("%s", buffer);
                    bzero(&buffer, BUFFER_SIZE);
                    break;
                } else
                    printf("%s", buffer);               
            }
            if (n == 0) {
                printf("Disconnection\n");
                break;
            } else if (n < 0) {
                error("recv");
            }
        }
    }

    return 0;
}

How the client works:

$ ./client -h 127.0.0.1 -p 6666 hello testdir .
host: 127.0.0.1 port: 6666 - connected
hello
        .
        ..
        file1
        file2
testdir
        Can't open the testdir
.
        .
        ..
        server
        server.o
        client
        hello
        client.c
        Makefile
        server.c
        client.o

Answer 1

I would change the protocol so that each command/reply has a tag, even if there is only one allowed in a given direction.

In this case, client-to-server would support:

list-files <dirname>

and server-to-client would support

begin-list-file <dirname> [more initial data - if possible, include the number of files here]
element-list-file <filename> [file specific data]
end-list-file <dirname> [more final data]

For whitespace, I prefer to strictly forbid \r and \t from appearing anywhere in the protocol. But if that's not acceptable to you, normalizing them as early as possible is best.

Answer 2

Recommend more validation of command line parameter:

Too easy for argv[1] to have a value like "-h" and then code proceeds with port with the value of 0.

// port = atoi(argv[1]);
char *endptr;
port = strtol(argv[1], &endptr, 10);
if (argv[1] == endptr || *endptr) Handle_NoConverisonExtraData();
if (port < PORT_MIN || port > PORT_MAX) Handle_BadPortNumber();

For consistency, use same error handler:

if(argc != 2) {
  error("usage: server port");
}

Answer 3

bzero is deprecated

bzero was deprecated in POSIX.2001 and is removed from the 2008 specification. You should use memsetinstead, which works just like bzero, except it has an additional parameter: the value to set each byte to. Source

bzero(&buffer, BUFFER_SIZE);
// becomes
memset(&buffer, 0, BUFFER_SIZE);

Unnecessary cast

bzero((char *) &server, sizeof(server));

It looks like you think bzero has a char * as it's first parameter. In reality, both bzero and memset has void * as the first parameter type. All other pointer types are automatically promoted to void * safely. For more info on this topic, check out Do I cast the result of malloc? Passing a struct sockaddr_in * works just fine.

bzero(&server, sizeof(server));
// or rather
memset(&server, 0, sizeof(server));

Formatting consistency

Code is easier to read when it follows certain formatting rules. You have some inconsistencies in your code. I've listed a couple I found below.

// Space after 'while'
while((opt = getopt(argc,argv,"h:p:")) != -1)
while ((dp = readdir(dir)) != NULL)

// Space between double paren
((dp = readdir(dir)) != NULL)
( (n = read(sockfd, buffer, BUFFER_SIZE)) > 0)

// Space before argument list
socket(AF_INET, SOCK_STREAM, 0))
setsockopt (sockfd, SOL_SOCKET, SO_REUSEADDR, &reuse,
        sizeof(reuse))