Feedback: Socket-Klasse

[EnginEx]

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;
}

[Kellerautomat]

- Es laeuft nur unter Windows.

[EnginEx]

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?

[cooky451]

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.

[EnginEx]

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?

[cooky451]

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.

[EnginEx]

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.

[Kellerautomat]

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

[cooky451]

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.

[EnginEx]

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.

[TyRoXx]

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?

[khhjk]

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.

[cooky451]

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.