Raw Text TCP Client

Yesterday I posted a request for a code review. After reviewing the code and the reviews, and after some sleep, I decided I can do better. In fact I logged on with the intent of deleting the question just as the first review was posted; so I was stuck with it.

This is pretty much the same program, but it is the result of much more effort. It even includes a thing I've never used before: select. To use the program, and the previous program, the user provides two arguments: the target IP and the target port. Contrary to the last program, this program terminates with ctrlc.

```
#include
#include
#include
#include
#include
#include
#include

#define BUFLEN 1024
#define STDIN 0
#define STDOUT 1

void sigint_handle(int dummy);
int connect_to(char *host, int port);
int transfer(int fd_in, char *buf, int buf_len, int fd_out);

// boolean flag for program continuation
volatile int run = 1;

// entry point of linux ELF
int main(int args, char **argv) {
int sd, ret_val = 0;
fd_set fds;
struct timeval tv;
char buffer[BUFLEN];

assert(args == 3);

signal(SIGINT, sigint_handle);

sd = connect_to(argv[1], atoi(argv[2]));

FD_ZERO(&fds);
tv.tv_sec = 0;
tv.tv_usec = 10000;
while (run) {
FD_SET(STDIN, &fds);
FD_SET(sd, &fds);
ret_val = select(sd + 1, &fds, NULL, NULL, &tv);
if (FD_ISSET(STDIN, &fds))
ret_val = transfer(STDIN, buffer, BUFLEN, sd);
else if (FD_ISSET(sd, &fds))
ret_val = transfer(sd, buffer, BUFLEN, STDOUT);
assert(ret_val == 0);
}
close(sd);
return 0;
}

// SIGINT handler
void sigint_handle(int dummy) {
run = 0;
}

// a very picky connect to tcp host function
int connect_to(char *host, int port) {
struct sockaddr_in addr;
int sd = socket(AF_INET, SOCK_STREAM, 0);
memset(&addr, 0, sizeof(addr));
assert(sd > -1);
addr.sin_family = AF_INET;
assert(inet_pton(AF_INET, host, &addr.sin_addr) > 0);
ad

A few notes:

-
As Jamal noted in the comments, your definitions of STDIN and STDOUT are somewhat useless. You can see here that using the given STDIN_FILENO and STDOUT_FILENO are already given those values. To counter your claim in the comments that they help readability, I would argue that they actually clutter up your code and should be removed.

-
Declaring run as a global variable isn't the best way to use signal handlers IMO. I would consider just testing the return value from signal for SIGINT in your while loop and quitting if that becomes true. I haven't tested this, but you shouldn't even need a sighandler_t for that.

-
The typical way that main() is written is as such:

int main(int argc, char **argv)

Note the argc, not the args. This actually stands for something ("argument count"), just like argv has a meaning as well ("argument vector").

-
All declarations should be on their own line. From Code Complete, 2d Edition, p. 759:


With statements on their own lines, the code reads from top to bottom,
instead of top to bottom and left to right. When you’re looking for a
specific line of code, your eye should be able to follow the left
margin of the code. It shouldn’t have to dip into each and every line
just because a single line might contain two statements.

-
Always initialize your values. Right now, you've only initialized one value to 0. Initialize them all. This is to safeguard against some possible weird bugs.

-
Don't assert anything that has to do with user input. Assertions are to be used to indicate the programmer's errors, not errors of the users. Instead, test if the input is what you'd like, and if it's not then print a message telling the user what they did wrong and return an error code.

-
Use braces with your if-else conditionals please. See the Apple goto fail as for a good reason why.

StackExchange Code Review Q#94710, answer score: 11