armandocivini / tp1

tp1

Amando Tomás Civini

https://github.com/ArmandoCivini/tp1.git

Para este TP se usó c en su totalidad. Haciendo uso de sus structs se crearon 5 tipos de datos abstractos con sus respectivas funciones para simplificar y construir el trabajo por bloques. 3 de estos tipos de datos abstractos cumplen la funcion de administrar la comunicacion entre el servidor y el cliente haciendo uso de sockets. En el siguiente diagrama se muestra los tipo de datos abstractos usados para esto y en donde son dependencia.

TDA Ahorcado

La primera parte del trabajo práctico que, se contruyó y se testeó, fue el TDA del ahorcado. Este comenzó simplemente como una palabra y la cantidad de intentos guardados en la estructura la cual mediante que se escribía el codigo se fueron agregando campos necesarios como el largo de la palabra, las letras reveladas actualmente. Ambas palabras que se guardan en el dato yacen en el heap por lo cual cuenta con una función de destrucción. Finalmente éste cuenta con una función para probar un caracter a la vez, la cual devuelve si ha ganado o no, lo que va revelado de la palabra(y en el caso de perder la palabra entera) y la cantidad de intentos restantes. Tambien este TDA tiene la opcion comenzar otro juego y esto lo hace mediante la funcion nueva_palabra() la cual guarda la nueva palabra en el struct y reinicia el estado del juego. De esto hace uso el servidor al leer una nueva linea del archivo de texto.

TDA Servidor

Este TDA contiene toda la informacion que requiere el servidor para operar y se encarga de manejar los recursos como el Sockt y el Ahorcado y de manejar la logica del juego. Cuenta con una funcion de destruccion que se encarga tambien de destruir los TDA que Servidor maneja.

TDA Sockt

El TDA sockt esta diseñado para uso generico de un socket, por esto implementa solamente un inizializador, una funcion para enviar un mensaje otra para recivirlo y un destructor. Este TDA se usa tanto en el cliente como en el servidor para enviar y recivir mensajes entre estos.

TDA Srv_sockt

Este TDA se encarga de crear el soccket del servidor que sera usado para escuchar conexiones y aceptarlas. El servidor hace uso de este TDA para hacerse presente y a medida que va podiendo procesar clientes usa la funcion de aceptar de este TDA para formar conexiones con ellos. La funcion de aceptar devuelve un file descriptor para ser usado en un TDA Sockt. Este TDA tambien cuenta con funcion de destruccion.

TDA Sockt_connect

El TDA Sockt_connect es usado por el cliente para inicializar la informacion de adress del servidor al que se quiere conectar para asi conectarse con este. la funcion sockt_connect_conection() tambien devuelve un file descriptor para ser usado con un TDA Sockt y asi intercambiar informacion con el servidor.

tp1's Issues

Mismo problema que en el socket del cliente

tp1/common_sockt_srv.c

Lines 59 to 68 in 0ed709f

	int sockt_srv_accept(Sockt_srv *skt){
	struct sockaddr_in *cli_act;
	unsigned int len = (socklen_t)sizeof(cli_act);
	int new_fd;
	new_fd = accept(skt->fd,(struct sockaddr *)&cli_act, &len);
	if (new_fd == -1){
	perror(ACCEPT_ERROR_MSG);
	}
	return new_fd;
	}

Devolviendo el file descriptor, rompes el encapsulamiento del socket. Podes crearte un socket de comunicación acá mismo, recibiendolo con un puntero por parametro y pasandole new_fd.

Hace uso de shutdown

tp1/server_sockt.c

Lines 87 to 92 in 762e4db

	void sockt_srv_destroy(Sockt_srv *skt){
	if (skt->fd_act != -1){
	close(skt->fd_act);
	}
	close(skt->fd);
	}

Hace uso de la syscall shutdown() para mandar un mensaje de cierre del stream, antes de hacer el close. Por ej, si querés cerrar los dos streams llamas a la syscall con el parámetro SHUT_RDWR.

Más macros

tp1/client.c

Lines 5 to 18 in 762e4db

	void juego_print(char *pal, uint8_t intentos){
	printf("Palabra secreta: %s\n", pal);
	int intent_p = intentos;
	printf("Te quedan %d intentos\n", intent_p);
	printf("Ingrese letra: \n");
	}
	void terminar_partida_perder(char *pal){
	printf("Perdiste! La palabra secreta era: '%s'\n", pal);
	}
	void terminar_partida_ganar(){
	printf("Ganaste!!\n");
	}

Podes separar bind y listen en otras funciones

tp1/common_sockt_srv.c

Lines 15 to 57 in 0ed709f

	int sockt_srv_init(Sockt_srv *skt, char *port, int max_q){
	struct addrinfo hints;
	struct addrinfo *ptr;
	int err = 0;
	int fd;
	memset(&hints, 0, sizeof(struct addrinfo));
	hints.ai_family = AF_INET;
	hints.ai_socktype = SOCK_STREAM;
	hints.ai_flags = AI_PASSIVE;
	err = getaddrinfo(NULL, port, &hints, &ptr);
	if (err != 0) {
	perror(ADDINFO_ERROR_MSG);
	return ERROR_NO;
	}
	fd = socket(ptr->ai_family, ptr->ai_socktype, ptr->ai_protocol);
	if (fd == -1) {
	perror(CREAR_ERROR_MSG);
	freeaddrinfo(ptr);
	return ERROR_NO;
	}
	err = bind(fd, ptr->ai_addr, ptr->ai_addrlen);
	if (err == -1) {
	perror(BIND_ERROR_MSG);
	close(fd);
	freeaddrinfo(ptr);
	return ERROR_NO;
	}
	freeaddrinfo(ptr);
	err = listen(fd, max_q);
	if (err == -1) {
	perror(LISTEN_ERROR_MSG);
	close(fd);
	return ERROR_NO;
	}
	skt->fd = fd;
	return 0;
	}

Haces todo el trabajo del socket del servidor en el constructor. Un diseño un poco más inteligente sería modularizar.
Además no iteras la lista de getaddrinfo, cuando si lo haces en el socket de conexion del cliente...

Que pasa si no se pudo abrir el archivo?

tp1/server.c

Line 56 in 762e4db

FILE *fil = fopen(file, "r");

Checkea por favor todos los retornos de funciones que reserven memoria, como malloc(), getline() y fopen(). Ademas, por qué ahorras letras al escribir? Se más claro. fil, aho, intent, son todos snippets de tu código y no es nada claro leer nombres de variables o funciones así.

No uses buffers de tamaño fijo

tp1/server.c

Lines 48 to 57 in 762e4db

	char s[32];
	char *pal = s;
	size_t size = 10;
	char pal_revelada[32];
	ssize_t len;
	sockt_srv_init(&skt, (uint16_t)port, 100);
	int victorias = 0;
	int derrotas = 0;
	FILE *fil = fopen(file, "r");
	while ((len = getline(&pal, &size, fil)) != -1){

En esta sección que te marqué, es donde rompe el código en una de las pruebas privadas (y por eso no te da verde el sercom). getline() la podes utilizar para leer lineas de tamaño variable del archivo. Si una palabra en el mismo tiene un tamaño mayor de la que le pasaste como parametro a la funcion, esta va a tirar un SIGKILL. De nuevo, evita utilizar buffers de tamaño fijo cuando lo que vas a leer es de tamaño variable.

Utilizá los sockets como vimos en clase

tp1/server_sockt.c

Lines 10 to 28 in 762e4db

	void sockt_srv_init(Sockt_srv *skt, uint16_t port, int max_q){
	skt->fd_act = -1;
	struct sockaddr_in addr;
	skt->fd = socket(AF_INET, SOCK_STREAM, 0);
	if(skt->fd == -1){
	perror("inicialización del socket");
	exit(1);
	}
	memset((char *)&addr,0, sizeof(addr));
	addr.sin_family = AF_INET;
	addr.sin_port = htons(port);
	addr.sin_addr.s_addr = INADDR_ANY;
	socklen_t sockaddr_size = (socklen_t)sizeof(struct sockaddr);
	if ( bind(skt->fd,(struct sockaddr *)&addr, sockaddr_size) == -1 ){
	perror("union socket server");
	exit(1);
	}
	listen(skt->fd, max_q);
	}

Tenes que hacer uso de getaddrinfo(), iterar por la lista de struct addrinfo, abrir un socket, checkear que ese socket sea válido y establecer recien ahi el bind. Si algo de esto falla, pasas al próximo nodo de la lista, mediante el campo ai_next dentro del mismo struct. Por favor, volvé a mirar la clase de sockets y mirate los ejemplos que están acá, leelos detalladamente y rearma un tda common_socket para ambos, cliente y servidor. Este mismo procedimiento va para el cliente, con connect(). https://github.com/Taller-de-Programacion/clases/tree/master/sockets-mdipaola/src

Código repetido en tda Ahorcado

tp1/common_ahorcado.c

Lines 7 to 20 in 0ed709f

	void ahorcado_init(Ahorcado *ahorcado, const char *p, int intentos){
	int largo = strlen(p);
	ahorcado->len = largo;
	ahorcado->pal = malloc(largo+1);
	strncpy(ahorcado->pal, p, largo+1);
	ahorcado->intentos = intentos;
	ahorcado->intentos_totales = intentos;
	char *pal_escondida = malloc(largo+1);
	for (int i = 0; i < largo; ++i){
	pal_escondida[i] = '_';
	}
	pal_escondida[largo] = '\0';
	ahorcado->revelados = pal_escondida;
	}

.
Es igual al código de la función nueva_palabra(...). Idealmente creas un tda ahorcado "pelado" y usas la otra función para ir cargando las palabras, eliminando esa responsabilidad del constructor.
Esa función ademas debería tener como prefijo el nombre del tda (ahorcado_nueva_palabra).

Y esta macro?

tp1/client_sockt_connect.c

Line 9 in 0ed709f

#define h_addr h_addr_list[0]

Esta función es inmensa

tp1/server.c

Lines 45 to 76 in 762e4db

	void empezar_juego(char *file, int port, int intentos){
	Sockt_srv skt;
	Ahorcado aho;
	char s[32];
	char *pal = s;
	size_t size = 10;
	char pal_revelada[32];
	ssize_t len;
	sockt_srv_init(&skt, (uint16_t)port, 100);
	int victorias = 0;
	int derrotas = 0;
	FILE *fil = fopen(file, "r");
	while ((len = getline(&pal, &size, fil)) != -1){
	if (pal[0] == '\n'){
	continue; //si hay linea vacia sigue
	}
	pal[len-1] = '\0';
	ahorcado_init(&aho, pal, intentos);
	sockt_srv_accept(&skt); //acepta a 1 cliente por palabra
	crear_fill(pal_revelada, len-1);
	enviar_msg(&skt, (uint8_t)intentos, pal_revelada, (uint16_t)(len-1));
	if(juego_loop(&skt, &aho, intentos, (uint16_t)len)){
	++victorias;
	} else{
	++derrotas;
	}
	ahorcado_destroy(&aho);
	}
	fclose(fil);
	print_recrd(victorias, derrotas);
	sockt_srv_destroy(&skt);
	}

Acá, como en el resto del tp, faltan tdas. Falta mínimo un tda server que tenga una estructura con el resto de las estructuras guardadas. Esta todo demasiado desperdigado y hay un alto acoplamiento. Idealmente, el tda server tendría el socket aceptador, la estructura del juego, etc.

Podrías haber hecho todo con un send y no con 2

tp1/common_sockt.c

Lines 37 to 55 in 0ed709f

	int sockt_write(Sockt *skt, char *buf, size_t exp_len){
	int bytes = send(skt->fd, buf, exp_len, 0);
	if (bytes == -1){
	sockt_destroy(skt);
	perror(ERROR_MSG);
	return ERROR_NO;
	}
	int bytes_sum = bytes; //bytes enviados hasta ahora
	while (bytes_sum < exp_len){
	bytes = send(skt->fd, &buf[bytes_sum], exp_len-bytes_sum, 0);
	if (bytes == -1){
	sockt_destroy(skt);
	perror(ERROR_MSG);
	return ERROR_NO;
	}
	bytes_sum += bytes;
	}
	return 0;
	}

Idem en el receiver.

Agrego idea para refactorizar los sockets

Lo que pasa en el codigo ahora es que basicamente tenes repetido el send y recv en socket_client y socket_server. Debatiendo con otros docentes, concluimos que capaz quisiste hacer sockets de comunicacion separados del socket aceptador (del server). Entonces la idea que te propongo es esa, realizar un tda que sea socket_listener, que se encargue de hacer el accept digamos del lado del server, y despues hacer un socket_communication (los nombres al margen, pero la idea podria ser tener sockets aceptador y comunicador) en donde resuelvas el send y el recv (es decir, este socket de comunicacion seria algo compartido por ambos programas, y el socket aceptador idealmente sería propio del server). De esta manera, podrias desacoplar al socket aceptador del tda en el que está ahora y evitar codigo repetido. La misma idea la podes hacer para el connect (del lado del cliente, podrias tener un tda connection socket). Te mando un diagrama de clases de como seria la idea.

Nota final: 5

Podrias evitar el buffer `revelados` devolviendo el buffer que tenes en el tda ahorcado

tp1/common_protocolo.c

Lines 114 to 127 in 0ed709f

	bool servidor_juego_loop(Servidor *servidor, uint16_t len){
	char buf[CARACTER], revelados[len+1];
	bool ganaste = false;
	int intentos = servidor->intentos;
	while(ganaste == false && intentos < ULTIMO_BIT){
	sockt_read(servidor->skt, buf, 1);
	ganaste = servidor_jugar_letra(servidor, revelados, buf[0]);
	servidor_enviar_msg(servidor, revelados, len-1);
	intentos = servidor->intentos;
	}
	return ganaste;
	}

No hace falta copiar a un buffer estatico la palabra revelada. Con devolver el puntero que tiene guardado en el struct Ahorcado es suficiente, ya que lo podes reutilizar, evitando una copia.
Un buffer de un caracter solo es equivalente a un caracter (buf[CARACTER]) -> mejor usar un caracter.

La idea era separar al servidor y al cliente del protocolo

tp1/common_protocolo.c

Lines 35 to 43 in 0ed709f

	void servidor_init(Servidor *servidor, Sockt_srv *skt_srv,
	Ahorcado *ahorcado, uint8_t intentos){
	servidor->ahorcado = ahorcado;
	servidor->skt_srv = skt_srv;
	servidor->victorias = 0;
	servidor->derrotas = 0;
	servidor->intentos = intentos;
	servidor->skt = NULL;
	}

Acá llamaste al archivo "common_protocolo" pero metiste toda la logica del server (que además no es common ya el cliente no utiliza el código del server). Es similar a lo que tenias antes pero con otro nombre, lo cual no soluciona el problema de acoplamiento entre el protocolo y el cliente (hacerlo common de hecho lo empeora ya que degrada al diseño).

Cuando dije tda protocolo me referia a tener un archivo separado (una clase aparte del cliente y del servidor), común al cliente y al servidor, que se encargue de construir los mensajes a partir de los datos del juego (los caracteres jugados, los intentos, la palabra adivinada hasta el momento, etc). Cliente y Servidor deberian forwardearle a Protocolo todos los datos requeridos y protocolo deberia pasarlos a buffers.
Este trabajo se está haciendo, pero no está encapsulado, esta TODO desperdigado entre el server y el client. Cambiarle el nombre a un archivo no es diseñar.

Protocolo acoplado con lógica del juego.

tp1/server.c

Lines 25 to 37 in 762e4db

	bool juego_loop(Sockt_srv *skt, Ahorcado *aho, int intent, uint16_t len){
	char buf[1], revel[len+1];
	bool ganaste = false;
	while(ganaste == false && intent < 128){
	sockt_srv_read(skt, buf, 1);
	ganaste = ahorcado_probar(aho, buf[0], revel, &intent);
	if (intent == 0 || ganaste){ //checkea si juego terminó
	intent += 128;
	}
	enviar_msg(skt, (uint8_t)intent, revel, (uint16_t)(len-1));
	}
	return ganaste;
	}

Que significa 128? Obviamente, es 2^7, y es representativo del protocolo. Podés construirte un tda protocolo que se encargue de descifrar esta lógica de los mensajes (codificar/decodificar), y además que maneje los sockets de comunicacion. El cliente y el servidor podrian llamar a protocolo_enviar_mensaje() (o a varias funciones), pasandole el socket y los parametros que sean necesarios para enviar un mensaje, pasarlo a bytestream por asi decirlo. Al momento de recibir, el protocolo sería el que se encargue de decodificar los bytes y devolver o una estructura o información que le puede ser relevante al client o al server. La idea aca seria que ninguno maneje la parte low level de bytes y sockets, sino que el mismo protocolo sea el encargado. O si no queres pasarle el socket al protocolo tambien podrias implementarte un callback. El diseño va por tu cuenta.

Acá estas "reinstanciando" a un mismo tda

tp1/server.c

Lines 62 to 71 in 762e4db

	ahorcado_init(&aho, pal, intentos);
	sockt_srv_accept(&skt); //acepta a 1 cliente por palabra
	crear_fill(pal_revelada, len-1);
	enviar_msg(&skt, (uint8_t)intentos, pal_revelada, (uint16_t)(len-1));
	if(juego_loop(&skt, &aho, intentos, (uint16_t)len)){
	++victorias;
	} else{
	++derrotas;
	}
	ahorcado_destroy(&aho);

Conceptualmente, está mal llamar al constructor de un objeto (en este caso es un tda, pero pensemos en objetos) multiples veces (el objeto se construye y destruye una sola vez). El problema entonces es que tenes comportamiento en el constructor que está ligado al comportamiento dinámico del objeto. En este caso, las palabras del ahorcado cambian, entonces podrias tener una función que se encargue de liberar la memoria reservada para la palabra anterior (si es que habia anterior) y que reserve memoria para la nueva. Podria ser un ahorcado_cargar_proxima(). Lo mas importante de este punto es sacar comportamiento ligado al constructor. El destructor lo llamas cuando no vas a usar mas el objeto (en el mismo scope en el que llamaste al constructor e instanciaste el objeto). Si esto no te queda claro, repasa la clase de objetos en C++, la idea es la misma.

Cuidado con este `sockt_destroy`

tp1/common_sockt.c

Lines 18 to 21 in 0ed709f

	if (bytes == -1){
	sockt_destroy(skt);
	perror(ERROR_MSG);
	return ERROR_NO;

Tenes un posible double free en este caso. Si llamas a sockt_destroy cuando falla send (lo mismo pasa en el recv), cuando main se vaya de scope en el cliente se volverá a llamar sockt_destroy, causando un double free y crasheando el programa.

tp1/client.c

Line 104 in 0ed709f

sockt_destroy(&skt);

.
La solución sería dejar al destructor unicamente en main.

Este `malloc` es innecesario

tp1/common_protocolo.c

Line 130 in 0ed709f

char *pal_revelada = malloc(sizeof(char) * len);

Pensá que cuando cambias una palabra, en el tda ahorcado creas dos buffers en el heap, uno para mantener la palabra original y otro para mantener los caracteres revelados hasta el momento. Ese ultimo se lo podrías devolver al server mediante un puntero y este se encargaria de mandarlo al cliente, en vez de reservar un arreglo el mismo y ademas llenarlo de '_' (que nuevamente rompe el encapsulamiento ya que esa es lógica del juego y no del servidor).

Macros

tp1/server.c

Lines 39 to 43 in 762e4db

	void print_recrd(int victorias, int derrotas){
	printf("Resumen:\n");
	printf(" Victorias: %d\n", victorias);
	printf(" Derrotas: %d\n", derrotas);
	}

Todo lo que sea strings, constantes, etc, van a macros o a enums.

Rompes el encapsulamiento

tp1/client.c

Lines 97 to 101 in 0ed709f

	fd = sockt_connect_connection(&skt_connect);
	if (fd == -1){
	sockt_connect_destroy(&skt_connect);
	return ERROR_NO;
	}

Al devolver el file descriptor al cliente rompes el encapsulamiento del socket de conexión. Algo mejor seria haber creado desde adentro el Socket de comunicación, pasando por referencia una estructura Sockt.

Evitá el uso de `exit()`

tp1/server_sockt.c

Line 50 in 762e4db

exit(1);

La syscall exit() mata al proceso, lo cual en ciertas situaciones significa que el mismo no liberó la memoria reservada ni cerró los streams que abrió en algún momento. La idea es que ante alguno de estos casos, vos devuelvas un código de error (devolver un numero negativo por lo general indica algun error, lo podés poner en una macro). Ante un error, dependiendo el tipo que sea, puede que tengas que cerrar el programa, pero antes asegurate de liberar TODOS los recursos. Este concepto se conoce como graceful exit. En este caso si falla el recv o el send, estás perdiendo toda la memoria, y esto no es ideal.

El tda socket no debería depender del servidor o del cliente

tp1/server_sockt.h

Lines 11 to 14 in 762e4db

	typedef struct Sockt_srv{
	int fd;
	int fd_act;
	} Sockt_srv;

Tenes sockets para el server y el client, que no son necesarios. Podes tener un common_socket que pueda ser utilizado en ambos programas. El socket aceptador (que es el que utilizará el server) estará entonces en un tda propio del server.

Reentrega

Procurá arreglar el socket y unificarlo. Hacete el tda protocolo. Checkea TODOS los mallocs, poniendo códigos de error si algo falla y resolve de otra manera como handlearlos (no con exit(1)). Pensa en los tda client y server, que tengan la estructura socket y que se comuniquen con el protocolo para enviar los mensajes y recibirlos (que el protocolo pueda codificar y decodificar), modularizá en más funciones. Escribi bien los nombres de las variables (nada de aho, intent), se prolijo, no uses buffers fijos para leer tamaños variables y utiliza macros. Despues, agrega al informe las correciones realizadas. Al informe tambien le falta, documenta un poco los tdas que utilizas. Cualquier consulta pregunta por discord. Es muy importante que arregles todos los issues para poder aprobar el tp.

Al tda protocolo

tp1/server.c

Lines 8 to 16 in 762e4db

	void enviar_msg(Sockt_srv *skt, uint8_t intentos, char *pal, uint16_t len){
	char *intent, len_pal[2];
	intent = (char *)&intentos;
	uint16_t len_pal_ns = htons(len); //conversion x endianess
	memcpy(len_pal, &len_pal_ns, 2);
	sockt_srv_write(skt, intent, 1);
	sockt_srv_write(skt, len_pal, 2);
	sockt_srv_write(skt, pal, (size_t)len);
	}

No está encapsulado el comportamiento del protocolo

tp1/client.c

Lines 31 to 51 in 0ed709f

	uint8_t adivinar_char(Sockt *skt, char *c, char **palabra){
	//char *palabra = NULL;
	uint8_t intentos;
	sockt_write(skt, c, 1);
	int err = protocolo_mensajes(skt, &intentos, palabra);
	if (err == -1){
	return ERROR_NO_U8;
	}
	if (intentos < ULTIMO_BIT){
	juego_print(*palabra, intentos);
	} else{
	if (intentos == ULTIMO_BIT){
	terminar_partida_perder(*palabra);
	} else{
	terminar_partida_ganar();
	}
	}
	return intentos; //para comunicar si partida terminó
	}

Tratar con bits era manejo del protocolo. Acá, en vez de devolver la cantidad de intentos estas directamente retornando el byte que te llego por el socket. La idea hubiese sido que el tda protocolo (el cual no está logrado) desarme ese byte en el bit de juego y los 7 bits de intentos. Esta información podría haber sido guardada incluso en una estructura para devolver al cliente. Como esta ahora, acoplas al protocolo con el cliente.

Checkea el fd antes de cerrar

tp1/common_sockt.c

Lines 57 to 60 in 0ed709f

	void sockt_destroy(Sockt *skt){
	shutdown(skt->fd, SHUT_RDWR);
	close(skt->fd);
	}

Podría ser que el file descriptor sea invalido y estarias cerrando cualquier cosa.

if (skt->fd != -1) {
    shutdown(skt->fd, SHUT_RDWR);
    close(skt->fd);
    skt->fd = -1;
}