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.

• Nikolay Bildeyko: thank you for a good advice. How I can parse it? So, I should sent to server for example a string list-files testDir then I'll parse it - find list-files in the string and after that I'll do action for this tag. All right?
• o11c: @NikolayBildeyko Read a line, cut off the trailing '\n'. Search for the ' ' and replace it with a '0円', what comes before is the key, what comes after (if anything) is an arg. You now have key and arg, key is now just "begin-list-file" etc, so use a hashmap/treemap/binary search lookup or just an if chain if there are few enough, call that code with the arg.

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))