Char array übergabe

Sone

sluz schrieb:

global erreichbaren Stack

Das ist es gerade. Du darfst nur Speicher freigeben, der im Heap allokiert wurde. Das der von deiner Instanz ( *this ) aber auch mit bspw. new allokiert wurde, garantiert keiner.

Ach ****, habe an das richtige gedacht und das falsche geschrieben - ist natürlich alles im Heap - und darauf wurde explizit beim coden geachtet das halt kein Problem damit ensteht.

Weiß jetzt aber auch was ihr meint und worauf ihr hinaus wolltet, danke euch

SeppJ

sluz schrieb:

Weiß jetzt aber auch was ihr meint und worauf ihr hinaus wolltet, danke euch

Nein, du weißt, worauf Sone hinaus will, aber nicht worauf ein vernünftiger Mensch hinaus möchte : Was passiert, wenn du später auf Member zugreifst? Bämm! Außerdem: Wie erfährt der Rest des Programms, dass das Objekt sich soeben selber zerstört hat? Der Aufrufer der Funktion hat immer noch seinen Pointer auf das Objekt und wird ihn wieder benutzen. Bämm!

Von dem was du anscheinend erreichen möchtest, suchst du eine Exception, keinen Selbstmord.

MAN! SeppJ!

Du weißt doch nicht mal wie mein Globaler Speicher aussieht, in dem meine Objekte liegen und schon urteilst du darüber, dass er hier und da Fehler verursacht. Aber hier einmal zur Info : auf die Objekte wir über eine statische Klasse zugegriffen in der überprüft wird ob das Objekt valide ist oder nicht.

Wenn du dennoch der Meinung bist, du könntest mich eines besseren belehren, dann lass uns wo anders unterhalten und hier nicht spammen

Sone

SeppJ schrieb:

sluz schrieb:

Weiß jetzt aber auch was ihr meint und worauf ihr hinaus wolltet, danke euch

Nein, du weißt, worauf Sone hinaus will, aber nicht worauf ein vernünftiger Mensch hinaus möchte

Das war einer der Gruende, wieso delete this beschissen ist.

DAS ist auch einer der Gruende wieso ich mich von der Irrlicht Engine distanziert habe.

http://irrlicht.sourceforge.net/docu/_i_reference_counted_8h_source.html#l00116

SeppJ

sluz schrieb:

MAN! SeppJ!

[...]

Wenn du dennoch der Meinung bist, du könntest mich eines besseren belehren, dann lass uns wo anders unterhalten und hier nicht spammen

Wenn du meinst, dass du mit

auf die Objekte wir über eine statische Klasse zugegriffen in der überprüft wird ob das Objekt valide ist oder nicht.

mich auch nur irgendwie überzeugen könntest, dass hier irgendetwas sinnvolles vorgeht, dann hast du gerade das Gegenteil erreicht. Hör auf mit dem C/Java-Denk (ich bin mir nicht mir sicher, welche Programmiersprache du gerade nachzumachen versuchst): C++ ist anders!

Einen brauchbarem Lösungsvorschlag erhältst du, wenn du verrätst, was du genau erreichen möchtest.

Eine Lösung habe ich schon lange, willst du wirklich sehen, was ich versuche aufzubauen? Aber na gut - Ich baue einen kleinen Instant-Messenger auf Basis von libev/GTK/polarssl.

Der delete this Teil ist ein Part vom libev loop indem die SSL Sockets angenommen werden und neue Objekte ( Clients ) mit allen Socket relevanten Informationen erstellt werden.

Es werden dann aber auch wirklich alle im Heap erstellt ( weswegen ich auch delete this bedenkenlos anwenden kann ), diese Pointer lagere ich in einer Static Klasse aus, damit ich vom Frontend auf die einzelnen Clients zugreifen kann.

Sone

sluz schrieb:

Es werden dann aber auch wirklich alle im Heap erstellt ( weswegen ich auch delete this bedenkenlos anwenden kann ),

Das ist ein sauhaessliches Design.

//Nachtrag
Achso und delete this wird ja nur aufgerufen wenn der Socket hinne ist - und wenn der Socket defekt ist, hat das ganze Objekt keinen Sinn mehr.

Diese Objekte arbeiten alle selbstständig in dem ev-Loop und werden sonst nicht von irgend einer anderen Klasse oder so überwacht - daher müssen die sich selber bereinigen wenn was nicht stimmt.

UND

Ich dachte, ein halbwegs geistlich labiler Programmierer wuerde ALLERHOECHSTENS im Destruktor delete this; schreiben. Alles andere ist Wahnsinn.

Welchen Sinn macht es im destructor delete this aufzurufen wenn das Objekt in dem Moment schon zerstört wird? Gerade das erscheint mir unlogisch zu sein.

Sone

sluz schrieb:

Gerade das erscheint mir unlogisch zu sein.

Ja, da hab ich dank einem Denkfehler Meilenweit verfehlt. Entschuldige.
Trotzdem ist es haesslich.

Sone,
schau dir mal bitte den Code hier an - http://www.skitoy.com/p/writing-an-echo-server-in-libev-and-c/375

Zeile 107 & 159 wären da relevant - kannst mir dann auch gerne sagen, dass das auch hässlicher Code ist, aber dann würde ich sehr gerne mal sehen wie "schöner" Code aussieht und wie man es lösen könnte.

Sone

sluz schrieb:

Zeile 107 & 159 wären da relevant - kannst mir dann auch gerne sagen, dass das auch hässlicher Code ist, aber dann würde ich sehr gerne mal sehen wie "schöner" Code aussieht und wie man es lösen könnte.

Ohne das jetzt so anzusehen, wuerde ich falls eine Heap-Allokation wirklich noetig ist einfach Smart-Pointer statt rohe Pointer bei der Instanziierung nehmen.

Also statt

EchoInstance* ptr = new EchoInstance(...);
//das
std::unique_ptr<EchoInstance> ptr( new EchoInstance(...) );

Nennt sich RAII

Smartpointer schön und gut - aber das ändert leider nichts daran, dass sich die Objekte selber bereinigen müssen wenn der Socket im Objekt hinne ist.

Und gerade an dem Punkt sehe ich keine andere Lösung als sich selber zu Töten - sonst hat ja keiner drauf Zugriff um zu testen ob das Objekt noch in Takt ist oder nicht. Muss halt von innen passieren.

Sone

sluz schrieb:

Smartpointer schön und gut - aber das ändert leider nichts daran, dass sich die Objekte selber bereinigen müssen wenn der Socket im Objekt hinne ist.

.....

Schau mal, was Smartpointer sind.

Athar

std::unique_ptr<EchoInstance> ptr( new EchoInstance(...) );

Und danach? Lässt du den Client in der nächsten Zeile sterben, wenn die Funktion zurückkehrt?

Prinzipiell kann man delete this; zwar machen, aber dann sollte der Konstruktor nicht öffentlich sein. Stattdessen bietest du eine statische create()-Methode an, die eine Instanz korrekt erzeugt.

Sone

Athar schrieb:

Und danach? Lässt du den Client in der nächsten Zeile sterben, wenn die Funktion zurückkehrt?

Ich deklariere ihn da, wo ich ihn brauche... wenn er weiterleben soll, wird das ueber Move-Semantik geklaert ( return unique_ptr<..>(..); ) oder ueber einen shared_ptr .

Auf jeden fall so, dass

semantisch garantiert wird dass der Speicher freigegeben wird
Kein delete this im Code steht.

Da Sone hier ja den Fundamentalisten spielt, will ich im Gegensatz zu ihm mit mehr Argumenten antworten.

In der C++ FAQ sind die Bedingungen aufgeführt, die erfüllt sein müssen, um delete this; einzusetzen. Hinweisen möchte ich insbesondere auf Regel 3:

3.) You must be absolutely 100% positively sure that the rest of your member function (after the delete this line) doesn't touch any piece of this object (including calling any other member functions or touching any data members).

Jetzt dein Code:

void sslSocketS::recvSocket ( char *data ) {

    if ( !validSocket() ) {
        info ( "Socket ist broken!" );
        delete this;
    };
    ...
    ssize_t size = ssl_read ( m_ssl, (unsigned char*) data, MAXRECV + 1 );
    ...
}

Auch wenn man nur vermuten kann, da du deine Klassendeklaration nicht angegeben hast, würde ich behaupten, dass ssl_read() und m_ssl Member deiner sslSocketS -Klasse sind.
Damit hat dein Code, für den Fall, dass delete this; ausgeführt wird, undefiniertes Verhalten.
Ich glaube auch, dass dein gepostetes Beispielprogramm nicht korrekt ist. Dort wird in der Funktion read_cb() delete this; aufgerufen. Dem Aufruf von read_cb() in Zeile 60 folgt in Zeile 65 der Zugriff auf die Membervariable write_queue . Also auch hier potentiell undefiniertes Verhalten.

Ich hoffe, du siehst ein, wie gefährlich der Gebrauch von delete this; ist und solltest es deshalb komplett vermeiden. Wie schon angesprochen lassen sich selbst die meisten Vorkommen von delete vermeiden, in dem man RAII zur Resourcenvewaltung einsetzt.

IrgendeinName

sluz schrieb:

Eine Lösung habe ich schon lange, willst du wirklich sehen, was ich versuche aufzubauen? Aber na gut - Ich baue einen kleinen Instant-Messenger auf Basis von libev/GTK/polarssl.

Der delete this Teil ist ein Part vom libev loop indem die SSL Sockets angenommen werden und neue Objekte ( Clients ) mit allen Socket relevanten Informationen erstellt werden.

Es werden dann aber auch wirklich alle im Heap erstellt ( weswegen ich auch delete this bedenkenlos anwenden kann ), diese Pointer lagere ich in einer Static Klasse aus, damit ich vom Frontend auf die einzelnen Clients zugreifen kann.

Also hat die statische Klasse (ich nenn diese einfach die Verwalterklasse) ein Pointer-Array, in dem die Sockets sind, und wenn die Sockets nicht funktionieren, lässt du sie mit delete this zerstören? Stell mich richtig, wenn ich dein Design nicht ganz durchschaue.

hustbaer

Jo, das ist übel.
Nach delete this; gehört alsbald ein return; . Zumindest - wie ja schon aus der Parashift FAQ zitiert wurde - darf man danach keine Member mehr angreifen.

Und natürlich stellt sich die Frage - wie ja auch schon geschrieben wurde - ob denn das restliche Programm "mitbekommt" dass das sslSocketS Objekt jetzt futsch ist.

----

Aber weiter (Anmerkungen als Code-Kommentar)...

void sslSocketS::recvSocket ( char *data ) { 

    if ( !validSocket() ) { 
        info ( "Socket ist broken!" ); 
        delete this;
        // Hier fehlt wie gesagt ein return;
    }; 

    // data ist ein Parameter - wieso tun wir das Array dann löschen und neu anlegen?
    // Und wie soll der Rest des Codes mitbekommen dass der data Zeiger geändert wurde?
    if ( data != NULL )
        delete[] data; // Immer neue Zeile nach if/for/while/... und schön Einrücken bringt Karma-Punkte
    data = new char[ MAXRECV + 1 ]; 
    memset ( data, 0, MAXRECV + 1 ); 
    ssize_t size = ssl_read ( m_ssl, (unsigned char*) data, MAXRECV + 1 );  // Siehe unten

    if ( size == POLARSSL_ERR_SSL_PEER_CLOSE_NOTIFY ) { 
        delete[] data; // Hm. Hier also "delete data;" - wieso dann oben beim "if ( !validSocket() )" kein "delete data;" ?
        data == NULL; // hier ist ein "=" zuviel. Davon abgesehen dass das Nullsetzen eines Parameters auch keiner mitbekommen wird
        info ( "Client disconnected" ); 
        delete this; 
        // Hier folgt zwar nix mehr, aber ein return; wäre trotzdem nett
    } 
    else if ( size == POLARSSL_ERR_NET_CONN_RESET ) { 
        delete[] data; 
        data = NULL; 
        warning ( "Reset by Peer!" ); 
        delete this; 
        // Hier folgt zwar nix mehr, aber ein return; wäre trotzdem nett
    }
    // Nu' haben wir schön size auf Fehler gecheckt - aber wie bekommt der Aufrufer mit wie viel empfangen wurde?
    // Wenn wir [c]MAXRECV[/c] statt [c]MAXRECV + 1[/c] an [c]ssl_read[/c] übergeben würden,
    // dann könnte man wenigstens noch davon ausgehen dass es immer ein Nullterminierter String ist (der könnte dann höchstens zu kurz ausfallen, falls der Client mal ein Nullbyte schickt, aber zumindest wäre kein Bufferoverrun möglich).
    // Aber so... übel.
}

OK, weiter...

Die Sache mit "delete this;" und einer globalen Verwaltungs-Instanz ist ja grundsätzlich noch OK, WENN die Klasse sich im Destruktor brav bei der Verwaltungs-Instanz "abmeldet".

Trotzdem bleibt die Frage, ob es OK ist wenn recvSocket einfach delete this; macht. Kommt die Aufrufende Stelle damit klar? Die kann das schliesslich nur mitbekommen, wenn sie nach dem Aufruf von recvSocket bei der Verwaltungs-Instanz nachfragt ob das Objekt noch registriert ist. Dazu muss entweder ein Zeiger auf das Objekt übergeben werden (was UB ist: Zeiger auf nicht-mehr-existierende Objekte sind ganz einfach ungültige Zeiger, und ungültige Zeiger übergeben/vergleichen/... ist eben UB). Oder aber der Socket muss anhand einer ID oder ähnlichem identifiziert werden. Auf jeden Fall wäre es langsam.

Und wie gesagt die Geschichte mit dem data Parameter... ich sehe da keine Möglichkeit wie das jemals funktionieren sollte.

Vorschlag:

char const* GetPolarSSLErrorMessage(int errorCode) {
    switch (errorCode) {
    case POLARSSL_ERR_SSL_PEER_CLOSE_NOTIFY:
        return "Client disconnected";
    case POLARSSL_ERR_NET_CONN_RESET:
        return "Reset by Peer!";
    default:
        return "Unknown error";
    }
}

void sslSocketS::breakSocket(char const* message) {
    info(message);
    m_broken = true;
}

bool sslSocketS::isBroken() { 
    if (!m_broken && !validSocket())
        breakSocket("Socket ist broken!");
    return m_broken;
}

size_t sslSocketS::recvSocket(char* data, size_t size) { 
    if (isBroken() || size == 0)
        return 0;
    assert(data != 0);

    int const rc = ssl_read(m_ssl, reinterpret_cast<unsigned char*>(data), size);
    if (rc < 0) {
        breakSocket(GetPolarSSLErrorMessage(rc));
        return 0;
    }
    else
        return static_cast<size_t>(rc);
}

Das Löschen und De-Registrieren verlagerst du dann an die Stelle die recvSocket aufruft. Wenn du sämtliche sslSocketS Funktionen mit einem if (isBroken()) nix tun und passenden Wert zurückgeben ausstattest musst du auch nicht nach jedem Aufruf einer sslSocketS Memberfunktion prüfen ob der Socket jetzt im Eimer ist, sondern u.U. sogar nur an einer einzigen Stelle.

Wenn du meinst dass es anders schöner/besser/toller wäre, dann zeig mal etwas mehr Code her. z.B. eben wo und wie recvSocket nun wirklich aufgerufen wird.

EDIT: OK, gerade erst gesehen... die checkSocket Funktion. Der Puffer ist dort NULL, weil du den Zeiger "by value" übergibst -- so kann die Aufrufende Funktion natürlich nicht mitbekommen was da passiert ist.
Aber wieso überhaupt in recvSocket jedes mal einen Puffer anlegen? Ist doch Verschwendung, leg den 1x an (als Member von ClientS ) und verwende ihn wieder.
Und... das sieht so aus als ob du ClientS von sslSocketS abgeleitet hättest. Das ist zwar eine Idee auf die Anfänger oft kommen, aber kein gutes Design. Ein Client (bzw. eine ClientConnection) "ist" kein Socket, sie "hat" einen Socket. -> sslSocketS sollte ein Member von ClientS sein, und keine Basisklasse.

EDIT2: Die Lösung für deine ganzen Probleme hier ist aber recht einfach: Lern C++ (bzw. Programmieren im Allgemeinen)