Impromptu TCP sender/receiver

Question

Every once in a while I need to get some random computer thing done; so I write something to do it for me. This is one of those. It needed to be able to send text to, and receive text from, a server - without blocking. I am asking for feedback with the premise of what kind of code this is: A one-use hack.

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <ctype.h>
#define BUFLEN 1024
volatile static int run = 1;
void sendx(int sd) {
 int sent, i;
 char buffer[BUFLEN], ch;
 do {
 i = 0;
 do {
 ch = getc(stdin);
 buffer[i] = ch;
 i++;
 } while (ch != '\n' && i < BUFLEN);
 sent = write(sd, buffer, i);
 } while (sent == i && run);
}
void recvx(int sd) {
 char nothing[] = { '?' };
 int len, i;
 char buffer[BUFLEN];
 do {
 len = read(sd, buffer, BUFLEN);
 i = 0;
 while (i < len) {
 if (isprint(buffer[i]))
 write(1, buffer + i, 1);
 else write(1, nothing, 1);
 i++;
 }
 } while (len > 0 && run);
 close(sd);
 nothing[0] = '\n';
 write(0, nothing, 1);
 fflush(stdin);
}
int main(int argc, char **argv) {
 int sd, pid;
 struct sockaddr_in addr;
 memset(&addr, 0, sizeof(struct sockaddr));
 assert(argc == 3);
 sd = socket(AF_INET, SOCK_STREAM, 0);
 addr.sin_family = AF_INET;
 assert(inet_pton(AF_INET, argv[1], &addr.sin_addr) > 0);
 addr.sin_port = htons(atoi(argv[2]));
 assert(sd > -1);
 assert(connect(sd, (struct sockaddr *)&addr, sizeof(addr)) > -1);
 pid = fork();
 assert(pid > -1);
 if (pid == 0) sendx(sd);
 else recvx(sd);
 run = 0;
 return 0;
}

Answer 1

Some of the logic is nicely encapsulated in functions sendx and recvx, but then main disappoints with its lump of code. It would be better to leave main in charge of parsing and validating the command line arguments, and extract the rest to functions with descriptive names.

In main the assert(argc == 3) should come before the first memset: no need to initialize memory if you might not use it.

In sendx, the variable i is used for two purposes: loop variable for reading input, and input size. If you extract the input reading to a function that returns the number of characters read, then in sendx you can store that in a variable with a descriptive name, and the logic of the main responsibility of this method will become clearer.

Answer 2

Run variable

It seems like you want to use the run variable to cause one process to stop if the other one stops. But since each process has its own copy of the run variable, it currently remains at value 1 and serves no purpose.

There are several ways to achieve interprocess communication if you really need this feature (e.g. shared memory, pipes, kill(), waitpid(), etc).

Answer 3

Code does not distinguish between EOF and some char.

getc(stdin) returns ant int in the range of unsigned char and EOF. This typically 257 difference values cannot be stored uniquely in a char.

// char ch;
int ch;
...
 ch = getc(stdin);
 // add
 if (ch == EOF) break;
...
} while (ch != '\n' && i < BUFLEN);

---

fflush(stdin);, although defined on some systems in undefined behavior as stdin is not an output stream nor an update stream .

If stream points to an output stream or an update stream ... the fflush function causes any unwritten data ... to be delivered ...; otherwise, the behavior is undefined.
"7.21.5.2 The fflush function"

---

I would expect the test of run to occur at the beginning of the loop. (2 places)

---

Niche concern: is...() functions are defined only for values of the range of unsigned char and EOF. Current code may fail on reading data with the value of 255 (Some encodings: 'ÿ'), which would likely save in a char as a -1. isprint(-1) is likely false as isprint() may see that as print(EOF). See https://stackoverflow.com/a/25382347/2410359

char buffer[BUFLEN]; // or use unsigned char buffer[BUFLEN]
if (isprint((unsigned char) buffer[i]))

---

Minimize use of magic numbers:

// write(1, nothing, 1);
write(1, nothing, sizeof nothing);

---

(削除) recvx() and sendx() both use sd asynchronously to each other. When recvx(int sd) closes sd, sendx() may not know that and need to. Suggest passing the address of sd so both routines access the same index which should be set to a invalid index after recvx(int sd) closes it. This volatile value could then be tested as sendx() needs it. (削除ここまで) per @JS comment.

• JS1: What you said about setting sd to an invalid index won't work because recvx() and sendx() are being run in two different processes.
• chux: @JS Agreed about sd.

Answer 4

Don't use assert for error checking! Those statements will be removed when the code is compiled with optimisation turned on. assert is meant as a "sanity check" for the programmer - use it to test for coding error, not errors in environment or user. In it's current state, this program would crash if the user forgets an argument if optimisation is turned on. assert is controlled by the macro NDEBUG.

I recommend switching to using exit, and preferably a message describing the error. Don't leave the user guessing. perror is convient for when system calls and standard function fail.