3. Char array übergabe

Char array übergabe

  • Mod

    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.

    0
  • ?

    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.

    0
  • S

    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.

    0
  • ?

    //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.

    0
  • S

    sluz schrieb:

    Gerade das erscheint mir unlogisch zu sein.

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

    0
  • ?

    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.

    0
  • S

    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 😉

    0
  • ?

    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.

    0
  • S

    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.

    0
  • A
     

    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.

    0
  • S

    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.
    0
  • ?

    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.

    0
  • I

    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.

    0

  • 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) 🤡

    0
Anmelden zum Antworten