3
\$\begingroup\$

Code reviewers, I have changed code whatever be suggested by code reviewer and I want you to give me some feedback on the basis of my new code.

I have written TCP server client code to send data of any size (1 byte - 200KB) in a loop.

The client (A) sends data to the server (B). B then sends the same data back to A. A matches the sent data to the data received. If the data does not match, control flow will break from the while-loop. If the data matches, the loop will continue to 200K bytes and then break successfully.

This question is a follow-up question on Send and Receive more than 64KiB over TCP/IP socket

Server code (server.c):

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>
#include <unistd.h>
#include <sys/select.h>
#include <errno.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <netinet/tcp.h>
#include <stdbool.h>
#include "init.h"

#define MAX_CLIENT 20000
#define SIZE 220000

int server_listen(int argc, char **argv);

int main(int argc, char *argv[])
{
    int ret, accept_desc, n, ssd;
    unsigned int bytes = 1;
    unsigned char *buf;
    fd_set rfd;

    ssd = server_listen(argc, argv); // --- return fd from listen()
    n = ssd;

    FD_ZERO(&rfd); // --- Clears a set
    FD_SET(ssd, &rfd); // --- Add a given file descriptor from a set

    while (true) {
        FD_ZERO(&rfd);
        FD_SET(n, &rfd);
        ret = select(n + 1, &rfd, NULL, NULL, NULL); // --- Waiting until one or more of the file descriptors become "ready"

        if (ret < 0) {
            perror("Error in select\n");
            close(n);
            exit(EXIT_FAILURE);
        }

        if (FD_ISSET(ssd, &rfd)) { // --- Tests to see if a file descriptor is part of the set
            accept_desc = server_accept(ssd); // ---Accept connection from client
            fprintf(stderr, "client %d is connected\n", accept_desc);
            if (n < accept_desc)
                n = accept_desc;
        }

        if (FD_ISSET(accept_desc, &rfd)) {
            bytes++;
            buf = receive(accept_desc, bytes); // --- Receive buffer from client
            if(buf == NULL) {
                fprintf(stderr, "Memory not allocated\n");
                close(accept_desc);
                exit(EXIT_FAILURE);
            }
            send_str(accept_desc, buf, bytes); // --- Send received buffer to client
            fprintf(stderr, "sent %d\n", bytes);
            free(buf); // --- Free the memory
        }
    }
}


int server_listen(int argc, char **argv)
{
    int ssd, flag = 1, ret, portno;
    unsigned int addrlen;
    struct sockaddr_in serv_addr;

    portno = get_option(argc, argv); // --- Get portno from user using getopt

    ssd = socket(AF_INET, SOCK_STREAM, 0); // --- socket creates an endpoint for communication

    if (ssd < 0) {
        perror("Error in socket");
        exit(EXIT_FAILURE);
    }

    ret = setsockopt(ssd, IPPROTO_TCP, TCP_NODELAY,
            (char *) &flag, sizeof(int)); // --- TCP_NODELAY used to disable nagle's algorithm
    if (ret != 0) {
        perror("Erro in tcp_nodelay");
        close(ssd);
        exit(EXIT_FAILURE);
    }

    ret = setsockopt(ssd, SOL_SOCKET, SO_REUSEADDR,
            (char *) &flag, sizeof(int)); // --- SO_REUSEADDR to reuse same port

    if (ret != 0) {
        perror("Error in reuseAddr");
        close(ssd);
        exit(EXIT_FAILURE);
    }

    serv_addr.sin_family = AF_INET;
    serv_addr.sin_addr.s_addr = INADDR_ANY;
    serv_addr.sin_port = htons((uint16_t)portno);
    bzero(&serv_addr.sin_zero, sizeof(serv_addr.sin_zero));
    addrlen = sizeof(serv_addr);

    ret = bind(ssd, (struct sockaddr *)&serv_addr, addrlen); // --- Bind assigns the address specified by addr to socket

    if (ret < 0) {
        perror("Error in binding");
        close(ssd);
        exit(EXIT_FAILURE);
    }
    fprintf(stderr, "Bind completed\n");
    //Listen
    ret = listen(ssd, MAX_CLIENT); // --- ready to accept incoming connection requests using accept()

    if (ret < 0) {
        perror("Error in listen");
        close(ssd);
        exit(EXIT_FAILURE);
    }
    fprintf(stderr, "listen\n");

    return ssd;
}

Client code (client.c):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <netdb.h>
#include <errno.h>
#include <sys/select.h>
#include <unistd.h>
#include <time.h>
#include <sys/types.h>
#include <netinet/tcp.h>
#include "init.h"

#define KB 1024
#define NO_EXIT_OR_ERROR 1
#define STATE_SEND 0
#define STATE_RECEIVE 1

unsigned int bytes;

struct hostent *server;

int client_connect(int argc, char **argv);

unsigned char *fill_n_send(int fd);

void receive_n_compare(int fd, unsigned char *buf_send);

int main(int argc, char *argv[])
{
    int fd, state = STATE_SEND, ret;
    struct timeval tv;
    unsigned char *buf_send;
    fd_set rfd;

    fd = client_connect(argc, argv); // --- Return fd from connect()
    tv.tv_sec = 0;
    tv.tv_usec = 100;

    FD_ZERO(&rfd); // --- Clears a set
    FD_SET(fd, &rfd);

    while(NO_EXIT_OR_ERROR) {
        FD_ZERO(&rfd);
        FD_SET(fd, &rfd); // --- Add a given file descriptor from a set
        ret = select(fd + 1, &rfd, NULL, NULL, &tv);

        if(ret < 0) { // --- Waiting until one or more of the file descriptors become "ready"
            perror("Select error");
            close(fd);
            exit(EXIT_FAILURE);
        }

        if(state == STATE_SEND) {
            buf_send = fill_n_send(fd);
            state = STATE_RECEIVE;

        }

        if (FD_ISSET(fd, &rfd)) { // --- Tests to see if a file descriptor is part of the set
            receive_n_compare(fd, buf_send);
            state = STATE_SEND;
        }
    }
}


unsigned char *fill_n_send(int fd)
{
    unsigned int start = 0;
    unsigned char *buf_send;

    buf_send = malloc(bytes + 2);
    if(buf_send == NULL) {
        fprintf(stderr, "Memory not allocated\n");
        close(fd);
        exit(EXIT_FAILURE);
    }

    while(start <= bytes){ // --- fill send buf like A, AA, AAA, AAAA ..... upto 200KB size and send one by one like first A then AA then AAA ...
        buf_send[start] = 'A';
        start++;
    }
    buf_send[++bytes] = 0;

    if(bytes >= 200*KB) {
        free(buf_send);
        close(fd);
        exit(EXIT_SUCCESS);
    }

    send_str(fd, buf_send, bytes + 1);

    return buf_send;
}


void receive_n_compare(int fd, unsigned char *buf_send)
{
    unsigned char *buf_recv;

    buf_recv = receive(fd, bytes + 1);

    if(buf_recv == NULL) {
        fprintf(stderr, "Memory not allocated\n");
        free(buf_send);
        close(fd);
        exit(EXIT_FAILURE);
    }

    if(strcmp((const char*)buf_send, (const char*)buf_recv) == 0){
        fprintf(stderr, "Match\n");
    } else {
        fprintf(stderr, "Unmatch %ld %ld\n", strlen((const char*)buf_send), strlen((const char*)buf_recv));
        close(fd);
        exit(EXIT_FAILURE);
    }

    free(buf_send);
    free(buf_recv);
}


int client_connect(int argc, char **argv)
{
    int fd1, ret, flag = 1, portno;
    struct sockaddr_in serv_addr;

    portno = get_option(argc, argv); // --- Get portno from user using getopt

    if(server == NULL) { // --- Check whether receive ip address of server
        fprintf(stderr, "Assign IP\n");
        exit(EXIT_FAILURE);
    }

    fd1 = socket(AF_INET, SOCK_STREAM, 0); // --- socket creates an endpoint for communication
    if (fd1 == -1) {
        perror("ERROR on creating Socket\n");
        exit(EXIT_FAILURE);
    }

    fprintf(stderr, "%d\n", fd1);
    fprintf(stderr, "socket created successfully.\n");
    bzero((char *) &serv_addr, sizeof(serv_addr));
    serv_addr.sin_family = AF_INET;
    bcopy((char *) server->h_addr, (char *)&serv_addr.sin_addr.s_addr,
         (long unsigned int)server->h_length);
    serv_addr.sin_port = htons((uint16_t)portno);

    ret = setsockopt(fd1, IPPROTO_TCP, TCP_NODELAY,
            (char *) &flag, sizeof(int)); // --- TCP_NODELAY used to disable nagle's algorithm

    if (ret != 0) {
        perror("Error in setsockopt");
        close(fd1);
        exit(EXIT_FAILURE);
    }

    if (connect(fd1, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) == -1) { // --- Connect the socket to the server
        perror("ERROR in Connecting");
        close(fd1);
        exit(EXIT_FAILURE);
    }

    printf("Client connected Successfully\n");

    return fd1;
}

Initial definition (init.c):

#include "init.h"
#include <stdio.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <netdb.h>
#include <stdbool.h>

#define SIZE 220000

struct hostent *server;

unsigned char *receive(int csd, unsigned int size)
{
    long int ret;
    unsigned char *buf = malloc(size), *buf1 = NULL;

    if(buf != NULL) {
        memset(buf, 0,size);
    } else {
        fprintf(stderr, "Memory not allocated\n");
        close(csd);
        exit(EXIT_FAILURE);
    }

    while(true) {
        ret = recv(csd, buf, size, 0);

        if(buf[ret-1] > 0) {
            buf1 = malloc(size);

            if(buf1 != NULL) {
                memset(buf1, 0,size);
            } else {
                fprintf(stderr, "Memory not allocated\n");
                free(buf);
                close(csd);
                exit(EXIT_FAILURE);
            }

            strcpy((char*)buf1, (const char*)buf);
            memset(buf, 0, size);
            continue;
        }

        if (ret < 0) {
            perror("Error in receive\n");
            close(csd);
            exit(EXIT_FAILURE);
        } else if (ret == 0) {
            fprintf(stderr, "client disconnected\n");
            close(csd);
        } else {

            if(buf1 != NULL) {
                strcat((char*)buf1, (const char*)buf);
                free(buf);
                return buf1;
            }
            return buf;
        }
    }

}


void send_str(int csd, unsigned char *buf, unsigned int size)
{
    long int ret;

    ret = send(csd, buf, size, 0);

    if (ret < 0) {
        perror("Error in send\n");
        close(csd);
        exit(EXIT_FAILURE);
    } else if(ret == 0) {
        fprintf(stderr, "Nothing to send\n");
        close(csd);
    }
}


int server_accept(int ssd)
{
    int accept_desc;
    struct sockaddr_in cli_addr;
    socklen_t cli_addr_len;

    accept_desc = accept(ssd, (struct sockaddr *)&cli_addr,
                  (socklen_t *)&cli_addr_len);

    if (accept_desc < 0) {
        perror("Error in accept\n");
        close(ssd);
        exit(EXIT_FAILURE);
    }

    return accept_desc;
}


int get_option(int argc, char **argv)
{
    int opt, portno = 0;

    while ((opt = getopt(argc, argv, "p:i:")) != -1) {

        switch (opt) {

        case 'p':
            portno = atoi(optarg);
            memset(&optarg, 0, strlen(optarg));
            break;

        case 'i':
            server = gethostbyname(optarg);
            memset(&optarg, 0, strlen(optarg));
            break;

        default: /* '?' */
            fprintf(stderr, "Usage: %s\n", argv[0]);
            exit(EXIT_FAILURE);;
        }
    }

    if (portno == 0) {
        fprintf(stderr, "Assign port number\n");
        exit(EXIT_FAILURE);
    }

    printf("portno = %d optind = %d\n", portno, optind);

    return portno;
}

Initial declaration (init.h):

#ifndef INIT_H
#define INIT_H

int get_option(int argc, char *argv[]);

unsigned char *receive(int csd, unsigned int size);

void send_str(int csd, unsigned char *buf, unsigned int size);

int server_accept(int ssd);

#endif // INIT_H

Makefile:

.PHONY: all server1 client1

all: server1 client1

server1: server.c init.c
        gcc -Wall -Wextra -Wconversion -pedantic -std=gnu11 server.c init.c -o s

client1: client.c init.c
        gcc -Wall -Wextra -Wconversion -pedantic -std=gnu11 client.c init.c -o c

server:
        ./s -p 13000

client:
        ./c -p 13000 -i 127.0.0.1
\$\endgroup\$
2
\$\begingroup\$

When calling any of the heap allocation functions: malloc() calloc() or realloc(), always check the returned value is not null to assure the operation was successful.

When communicating over a TCP/IP, connection, the data (especially after about 1650 bytes in a single packet) will break the data into multiple packets. Therefore your code needs to expect that kind of event. I suggest using a 'sliding window', in a loop, until all the data is sent/received.

The Makefile has rules named client and server but that Makefile, as written, will not execute those two executables (especially at the same time).

In the Makefile, the rules server1 and client1 do not list the header file init.h as a dependency, so if/when the header file is modified, the executables will NOT be rebuilt.

Regarding ret = listen(ssd, MAX_CLIENT);, for this to have ANY chance of handling 20000 clients each, the client should be passed to a 'thread pool' entry. As it is, only 1 client will be accepted at a time. All other clients will receive an error indication.

\$\endgroup\$
  • \$\begingroup\$ Hi @user3629249 can you please show me where I can't check (!=NULL). \$\endgroup\$ – ASid13 Jun 21 '18 at 5:08
  • 1
    \$\begingroup\$ I can't see any unchecked allocations - please be more specific which one you think is missing the necessary check. \$\endgroup\$ – Toby Speight Jun 21 '18 at 8:29
1
\$\begingroup\$

The Makefile could be improved:

  • Without a .DELETE_ON_ERROR target, Make won't remove outputs of commands that fail. Every Makefile should have this target.
  • server1 and client1 are marked as .PHONY, but they produce actual files (s and c respectively); these will be written even when up to date.
  • server and client are not marked as .PHONY, but their rules should be run even if such files exist.
  • server and client don't depend on their respective programs, so may invoke out-of-date code
  • The above points could be avoided with a clearer naming convention: I like to use nouns (or obvious filenames) for real targets and verbs for phony targets. In this case, it would be server and client for the programs and run-server and run-client to execute them.
  • We can make better use of built-in rules if we set CFLAGS appropriately.
  • Instead of compiling init.c twice, we could make both client and server depend on init.o. Then it will be compiled just once, and linked into both programs.

On the positive side, I'm pleased to see a good level of warning options enabled.


Here's my suggested replacement (I'm assuming GNU Make):

CC = gcc

CFLAGS = -std=gnu11
# Debugging
#CFLAGS += -g
# Optimisation
#CFLAGS += -O2
# Warnings
CFLAGS += -Wall -Wextra -Wconversion -pedantic

all: server client

server: server.o init.o
client: client.o init.o

server.o: init.h
client.o: init.h
init.o: init.h


run-%: %
    ./$< $(RUN_ARGS)

run-server: RUN_ARGS = -p 13000
run-client: RUN_ARGS = -p 13000 -i 127.0.0.1


clean::
    $(RM) client server *.o *~

.PHONY: all run-% clean
.DELETE_ON_ERROR:
\$\endgroup\$
  • \$\begingroup\$ Can you please tell me one thing, How can I use recv() with MSG_DONTWAIT flag so I can use it without error. Current I am working on this and I am facing issue with recv() although my send() works fine with MSG_DONTWAIT. \$\endgroup\$ – ASid13 Jun 21 '18 at 9:12
  • \$\begingroup\$ I haven't looked at the C code yet. \$\endgroup\$ – Toby Speight Jun 21 '18 at 9:22
  • \$\begingroup\$ ok no problem, If you know anything please let me know. thanks \$\endgroup\$ – ASid13 Jun 21 '18 at 9:44

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.