Feedback: Socket-Klasse



  • Hey,

    ich wollte hier mal fragen was man an dieser Klasse denn verändern/verbessen könnte.

    #pragma once
    
    #include <WinSock.h>
    #include <string>
    
    bool startWSA();
    void stopWSA();
    
    class Socket {
    
    public:
    
    	Socket();
    	Socket(const std::string protocol);
    	~Socket();
    
    	int listen();
    	int bind(const int port, const std::string addr);
    	int connect(const int port, const std::string host);
    	int accept(Socket &sock);
    	int sendData(const std::string &data) const;
    	int sendData(const char *data) const;
    	int recvData(std::string &data) const;
    
    private:
    
    	int	   error;
    	SOCKET _sock;
    
    	struct sockaddr_in sockAddr;
    	struct hostent	   *host;
    
    	enum { MAX_CONNECTIONS = SOMAXCONN, BUFF_SIZE = 256 };
    
    	//Socket(const Socket&);
    	//Socket& operator=(const Socket&);
    };
    
    #include "socket.h"
    
    bool startWSA() {
    
    	WSADATA wsaData;
    	WORD wVersionRequested = MAKEWORD(2, 2);
    	int error = WSAStartup(wVersionRequested, &wsaData);
    
    	if (error != 0) {
    		return false;
    	}
    	else {
    		return true;
    	}
    }
    
    void stopWSA() {
    	WSACleanup();
    }
    
    Socket::Socket() : _sock(socket(PF_INET, SOCK_STREAM, 0)) {}
    
    Socket::Socket(const std::string protocol) {
    
    	if (protocol == "UDP") {
    		_sock = socket(PF_INET, SOCK_DGRAM, 0);
    	}
    	else if (protocol == "TCP") {
    		_sock = socket(PF_INET, SOCK_STREAM, 0);
    	}
    	else {
    		throw std::runtime_error("Unsupported Protocol - socket() not called");
    	}
    }
    
    Socket::~Socket() {
    	closesocket(_sock);
    }
    
    int Socket::bind(const int port, const std::string addr) {
    
    	memset(&sockAddr, 0, sizeof(sockAddr));
    	sockAddr.sin_family      = AF_INET;
    	sockAddr.sin_addr.s_addr = inet_addr(addr.c_str());
    	sockAddr.sin_port        = htons(port);
    
    	error = ::bind(_sock, reinterpret_cast<struct sockaddr *>(&sockAddr), sizeof(struct sockaddr));
    
    	if (error == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int Socket::listen() {
    
    	error = ::listen(_sock, MAX_CONNECTIONS);
    
    	if (error == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	return 0;
    }
    
    int Socket::accept(Socket &listen_sock) {	
    
    	_sock = ::accept(listen_sock._sock, 0, 0);
    
    	if (_sock == INVALID_SOCKET) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int Socket::connect(const int port, const std::string hostname) {
    
    	memset(&sockAddr, 0, sizeof(sockAddr));
    	sockAddr.sin_family  = AF_INET;	
    	sockAddr.sin_port    = htons(port);
    
    	host = gethostbyname(hostname.c_str());
    
    	if (host != 0) { 
    		sockAddr.sin_addr = *reinterpret_cast<struct in_addr*>(host->h_addr);
    	}
    	else {
    		sockAddr.sin_addr.s_addr = inet_addr(hostname.c_str());
    	}
    
    	error = ::connect(_sock, reinterpret_cast<struct sockaddr*>(&sockAddr), sizeof(struct sockaddr));
    
    	if (error != 0) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int Socket::sendData(const std::string &data) const {
    
    	int bytes = send(_sock, data.c_str(), data.size(), 0);
    
    	if (bytes == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int Socket::sendData(const char *data) const {
    
    	int bytes = send(_sock, data, strlen(data), 0);
    
    	if (bytes == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int Socket::recvData(std::string &data) const {
    
    	char buffer[BUFF_SIZE];
    	int  bytes = 0;
    
    	data.clear();
    
    	do {
    		bytes = recv(_sock, buffer, sizeof(buffer) - 1, 0);
    
    		if (bytes == SOCKET_ERROR) {
    			return WSAGetLastError();
    		} 
    		else {
    			data.append(buffer, bytes);
    		}
    	} while (bytes > 0);
    
    	return 0;
    }
    


  • - alle Member außer _sock wären besser lokale Variablen

    • std::string per Referenz übergeben
    • enum statt String für Protokollunterscheidung verwenden
      - es gibt keine Methoden für UDP
      - umständliche if/else/return Konstruktionen
    • sendData ist falsch, weil send nicht alles auf einmal senden muss (EDIT: Hier korrekt, aber im nicht-blockierenden Modus würde es so nicht funktionieren)
      - der Empfangspuffer ist mit 256-1 Bytes sehr klein
      - warum glaubst du, dass das letzte Byte des Puffers nicht nutzbar wäre?
      - Kopieroperationen explizit ausschalten
      - eine swap -Methode hinzufügen
      - die Klasse movable machen
      - die Klasse aus dem globalen Namensraum rausnehmen
    • sendData(const char *data) ist nicht sehr nützlich, besser wäre sendData(const char *data, size_t length)
    • sendData und recvData sollten nicht const sein, denn sie verändern den internen Zustand des Objektes
    • recvData könnte besser receiveData heißen


  • So wie ich das sehe, hast Du einen blockierenden Socket. Dein sendData ist also in Ordnung, send sendet alles oder nichts auf einem blocking Socket.
    Aber Dein recvData bleibt hängen, bis am anderen Ende der Socket geschlossen und so die Verbindung abgebrochen wird, ist das so beabsichtigt?



  • Eine Klasse , die C-Funktionen wrappt, sollte einem vorallem Arbeit abnehmen. D.h. in Bezug auf den Anwendungszweck einzelne C-Funktionen in einer Methode zusammenfassen. Die Symmetrie von Senden und Empfangen sollte sich in der Parameterliste widerspiegeln. In diesem Fall tut es das. Gut! Leider mag ich auch andere Dinge senden, die eben kein nullterminierter String sind, die Variante mit const char* ohne Laengenangabe ist also falsch. Darueber hinaus wuerde ich std::vector<uint8_t> verwenden.



  • TyRoXx schrieb:

    - alle Member außer _sock wären besser lokale Variablen

    Wieso? Für eine optimale Kapselung? Ist das später nicht eher hinderlich, wenn ich meine Socket-Klasse mal in Threads nutzen möchte?

    TyRoXx schrieb:

    - es gibt keine Methoden für UDP

    Ja, da bin ich noch nicht zu gekommen. Ist aber geplant genau wie select(), blocking/nonblocking etc.

    TyRoXx schrieb:

    - umständliche if/else/return Konstruktionen

    Wie würdest du das lösen?

    TyRoXx schrieb:

    - eine swap -Methode hinzufügen
    - die Klasse movable machen

    Hab ich mal hinzugefügt, wäre aber nett wenn da nochmal jemand drüberschaut bin mit dem C++11-Standard noch nicht so wirklich vertraut.

    Darueber hinaus wuerde ich std::vector<uint8_t> verwenden.

    Habe da jetzt einen std::string genommen, reicht meiner Meinung nach vollkommen aus.

    Aber Dein recvData bleibt hängen, bis am anderen Ende der Socket geschlossen und so die Verbindung abgebrochen wird, ist das so beabsichtigt?

    Ja, aber je nach Anwendungsfall kann das wirklich nicht erwünscht sein, werde ich heute Abend ändern.

    Hier mal der überarbeitete Code:

    #pragma once
    
    #include <string>
    #include <WinSock.h>
    
    namespace network {
    
    	bool startWSA();
    	void stopWSA();
    
    	enum class protocol { TCP, UDP };
    
    	class Socket {
    
    	private:
    
    		SOCKET _sock;
    		int retval;
    
    		struct sockaddr_in sockAddr;
    		struct hostent	   *host;
    
    		enum { MAX_CONNECTIONS = SOMAXCONN, BUFF_SIZE = 4096 };
    
    		Socket(const Socket&);
    		Socket& operator=(const Socket&);
    
    	public:
    
    		Socket();
    		Socket(const protocol p);
    		Socket(Socket &&rhs);
    		~Socket();
    
    		int listen();
    		int bind(const int port, const std::string &addr);
    		int connect(const int port, const std::string &host);
    		int accept(Socket &sock);
    		int sendData(const std::string &data);
    		int sendData(const char *data, const size_t length);
    		int recvData(std::string &data);
    
    		friend void swap(Socket &lhs, Socket &rhs) throw();
    	};
    }
    
    #include "socket.h"
    
    bool network::startWSA() {
    
    	WSADATA wsaData;
    	WORD wVersionRequested = MAKEWORD(2, 2);
    	int retval = WSAStartup(wVersionRequested, &wsaData);
    
    	if (retval != 0) {
    		return false;
    	}
    	else {
    		return true;
    	}
    }
    
    void network::stopWSA() {
    	WSACleanup();
    }
    
    network::Socket::Socket() : _sock(socket(PF_INET, SOCK_STREAM, 0)) {}
    
    network::Socket::Socket(const protocol p) {
    
    	if (p == protocol::UDP) {
    		_sock = socket(PF_INET, SOCK_DGRAM, 0);
    	}
    	else if (p == protocol::TCP) {
    		_sock = socket(PF_INET, SOCK_STREAM, 0);
    	}
    	else {
    		throw std::runtime_error("Unsupported Protocol - socket() not called");
    	}
    }
    
    network::Socket::Socket(Socket &&rhs) : _sock(rhs._sock) {
    	rhs._sock = 0;
    }
    
    network::Socket::~Socket() {
    	closesocket(_sock);
    }
    
    int network::Socket::bind(const int port, const std::string &addr) {
    
    	memset(&sockAddr, 0, sizeof(sockAddr));
    	sockAddr.sin_family      = AF_INET;
    	sockAddr.sin_addr.s_addr = inet_addr(addr.c_str());
    	sockAddr.sin_port        = htons(port);
    
    	retval = ::bind(_sock, reinterpret_cast<struct sockaddr *>(&sockAddr), sizeof(struct sockaddr));
    
    	if (retval == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int network::Socket::listen() {
    
    	retval = ::listen(_sock, MAX_CONNECTIONS);
    
    	if (retval == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	return 0;
    }
    
    int network::Socket::accept(Socket &listen_sock) {	
    
    	_sock = ::accept(listen_sock._sock, 0, 0);
    
    	if (_sock == INVALID_SOCKET) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int network::Socket::connect(const int port, const std::string &hostname) {
    
    	memset(&sockAddr, 0, sizeof(sockAddr));
    	sockAddr.sin_family  = AF_INET;	
    	sockAddr.sin_port    = htons(port);
    
    	host = gethostbyname(hostname.c_str());
    
    	if (host != 0) { 
    		sockAddr.sin_addr = *reinterpret_cast<struct in_addr*>(host->h_addr);
    	}
    	else {
    		sockAddr.sin_addr.s_addr = inet_addr(hostname.c_str());
    	}
    
    	retval = ::connect(_sock, reinterpret_cast<struct sockaddr*>(&sockAddr), sizeof(struct sockaddr));
    
    	if (retval != 0) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int network::Socket::sendData(const std::string &data) {
    
    	retval = send(_sock, data.c_str(), data.size(), 0);
    
    	if (retval == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int network::Socket::sendData(const char *data, const size_t length) {
    
    	retval = send(_sock, data, length, 0);
    
    	if (retval == SOCKET_ERROR) {
    		return WSAGetLastError();
    	}
    	else {
    		return 0;
    	}
    }
    
    int network::Socket::recvData(std::string &data) {
    
    	std::string buffer;
    	buffer.resize(BUFF_SIZE);
    	data.clear();
    
    	do {
    		retval = recv(_sock, &buffer[0], buffer.size(), 0);
    
    		if (retval == SOCKET_ERROR) {
    			return WSAGetLastError();
    		} 
    		else {
    			data.append(&buffer[0], retval);
    		}
    	} while (retval > 0);
    
    	return 0;
    }
    
    void network::swap(Socket &lhs, Socket &rhs) throw() {
    
    	Socket tmp_sock;
    
    	tmp_sock._sock = lhs._sock;
    	lhs._sock	   = rhs._sock;
    	rhs._sock	   = tmp_sock._sock;
    }
    


  • - Es laeuft nur unter Windows.



  • Wenn ich das jetzt auf Linux portieren wollen würde, dann müsste ich doch bei fast jeder Methode sowas machen, oder?

    #ifdef OS_WINDOWS
       // Windows Code
    #else
      // Linux, Mac, whatever
    #endif
    

    Gibt es da auch eine elegantere Methode?



  • EnginEx schrieb:

    Wieso? Für eine optimale Kapselung? Ist das später nicht eher hinderlich, wenn ich meine Socket-Klasse mal in Threads nutzen möchte?

    Nein, wie kommst du denn da drauf?

    EnginEx schrieb:

    Habe da jetzt einen std::string genommen, reicht meiner Meinung nach vollkommen aus.

    Ein Socket der nur Strings empfangen kann, ja, sehr sinnvoll. 🙄

    Dass du in deiner swap Funktion einen neuen Socket erstellst, macht auch keinen Sinn. Der Gag an einer swap Methode ist eigentlich, dass du einfach die handels tauschst. Also:

    socket::swap(socket& rhs)
    {
      std::swap(handle_, rhs.handle_);
    }
    

    Vorausgesetzt deine Klasse hat nur ein handle als Member - was recht sinnvoll ist.



  • cooky451 schrieb:

    Nein, wie kommst du denn da drauf?

    Naja, angenommen in einem Thread wird die connect()-Methode mit entsprechenden Parametern aufgerufen. Daraufhin wird u.a. in sockAddr der Port und Host gespeichert. Wenn nach dem setzen und vor dem connect()-Aufruf in einem anderen Thread genau das gleiche mit anderem Port/Host geschieht und in sockAddr gespeichert wird verbindet sich Socket 1 nun woanders hin als ursprünglich gewollt, oder nicht?



  • EnginEx schrieb:

    cooky451 schrieb:

    Nein, wie kommst du denn da drauf?

    Naja, angenommen in einem Thread wird die connect()-Methode mit entsprechenden Parametern aufgerufen. Daraufhin wird u.a. in sockAddr der Port und Host gespeichert. Wenn nach dem setzen und vor dem connect()-Aufruf in einem anderen Thread genau das gleiche mit anderem Port/Host geschieht und in sockAddr gespeichert wird verbindet sich Socket 1 nun woanders hin als ursprünglich gewollt, oder nicht?

    Nö, jeder "Aufruf" einer Funktion bekommt seine eigenen lokalen Variablen. Wie sollte sonst rekursion funktionieren? "Thread sicherer" als lokale Variablen geht nicht. Aufpassen musst du da gerade bei nicht lokalen Variablen.



  • Dann verstehe ich aber nicht, was es bringen soll die sockaddr_in etc. zu lokalen Variablen zu machen. Ich sehe da keinerlei Vorteil, eher einen Nachteil, da direkt darauf zugegriffen werden kann.



  • Wozu braucht der Socket ueberhaupt die anderen Member als das Handle? Die sind fuer die Identifikation irrelephant.



  • EnginEx schrieb:

    Ich sehe da keinerlei Vorteil,

    Sie sind lokaler und liegen nicht die ganze Zeit sinnlos im Speicher rum?

    EnginEx schrieb:

    eher einen Nachteil, da direkt darauf zugegriffen werden kann.

    Hä? Erklär mal.



  • Vergesst alles was ich gesagt habe. Missverständnis meinerseits, irgendwie dachte ich bei lokalen Variablen an sowas:

    namespace network {
    
       struct sockaddr_in sockAddr;
       struct hostent     *host;
    
       bool startWSA();
       void stopWSA();
    
       class Socket { /* ... */ };
    }
    

    ... was natürlich Blödsinn ist. Sorry.



  • cooky451 schrieb:

    EnginEx schrieb:

    Habe da jetzt einen std::string genommen, reicht meiner Meinung nach vollkommen aus.

    Ein Socket der nur Strings empfangen kann, ja, sehr sinnvoll. 🙄

    Ein string ist wie ein vector<char> eine Folge von Bytes. Was kann man denn außer Bytes noch empfangen?



  • TyRoXx schrieb:

    cooky451 schrieb:

    EnginEx schrieb:

    Habe da jetzt einen std::string genommen, reicht meiner Meinung nach vollkommen aus.

    Ein Socket der nur Strings empfangen kann, ja, sehr sinnvoll. 🙄

    Ein string ist wie ein vector<char> eine Folge von Bytes. Was kann man denn außer Bytes noch empfangen?

    Man kann es evtl. nur anders interpretieren (z.B. als int, double,...), aber das geht mit einem std::string(buf, len) auch.



  • TyRoXx schrieb:

    Ein string ist wie ein vector<char> eine Folge von Bytes. Was kann man denn außer Bytes noch empfangen?

    Ein std::string modelliert einen String. Ein vector<char> modelliert eine Folge von char. Wenn ich eine Folge von char/byte empfangen will, nehme ich einen vector<char>. 🙄
    Und nein, ein String ist semantisch nicht das gleiche wie eine Folge von char.


Log in to reply