5
\$\begingroup\$

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
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Nikolay Bildeyko Jun 25 '15 at 14:36
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – o11c Jun 25 '15 at 18:03
3
\$\begingroup\$

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");
}
\$\endgroup\$
2
\$\begingroup\$

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))
\$\endgroup\$

Your Answer

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

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