buffer probleem socket programmeren

[High Quality king]

Beste tweakers,

Ik heb een stukje c code geschreven dat prima werkt (hoera), totdat ik de buffer verhoog. Het ontvangen bestand is dan groter dan het origineel en dus corrupt.

Het werkt tot ongeveer 8kb. Iemand enig idee?

code:

uint64_t sendBuffer(struct file *openFile, struct tcpHandle *connection) //check
{
    uint64_t size, result = 0;
    
    sendSize(openFile->properties.st_size, connection);
    
    char buffer[TRANSFERBUFFER];
    
    while ((size = read(openFile->handle, buffer, TRANSFERBUFFER)) > 0)
    {
        result += send(connection->handle, buffer, size, 0);
        //printf("size : %lld result %lld\n", size, result);
    }
    close(openFile->handle);
    
    if( result != (size = recvSize(connection)) )
    {
        printf("filesize %llu and sendsize %llu do not match",size, result);
    }
    
    return result;
}

uint64_t recvBuffer(struct file *openFile, struct tcpHandle *connection) // check
{
    uint64_t size = recvSize(connection), result = 0;
    
    char buffer[TRANSFERBUFFER];
    
    while(size > TRANSFERBUFFER)
    {
        size -= recv(connection->handle,buffer,TRANSFERBUFFER,0);
        result += write(openFile->handle, buffer, TRANSFERBUFFER);
    }
    if(size > 0)
    {
        recv(connection->handle, buffer, size, 0); 
        result += write(openFile->handle, buffer, size);
    }
    close(openFile->handle);
    
    sendSize(result, connection);
    
    return result;
}

[Sircuri]

ik vermoed dat het in de constructie zit op regel 32. Welke garantie heb je dat je volledige buffer volgeschreven wordt? je schrijft je volledige buffer weg naar de openFile-handler op regel 33 terwijl je helemaal niet weet hoeveel bytes er zijn ingelezen in de buffer. Je zal denk ik TRANSFERBUFFER op regel 33 moeten vervangen door de hoeveelheid bytes ingelezen middels de recv functie op regel 32.

update: op regel 9 en 11 doe je het wel goed voor het schrijven van het bestand naar een socket.

[ Voor 11% gewijzigd door Sircuri op 30-01-2014 10:07 ]

[High Quality king]

Uhm,

Op regel 26 krijgt hij de volledige grote van het bestand (size).

zolang size hoger is dan de transferbuffer blijft hij op de transferbuffer wachten (3e parameter van recv), als het aantal ontvangen bytes size -= recv kleiner wordt dan de buffer dan schrijft hij het restand weg in de if op regel 35.

De TRANSFERBUFFER == de hoeveelheid ingelezen bytes en de buffer is precies zo groot.

Het lukt mij 700 MB over te pompen met een buffer grote van 8 kb, maar vanaf 16 kb krijg ik corruptie en is het ontvangen bestand (marginaal) groter dan het verzonden bestand. Ik zat in de uiteindelijke toepassing aan iets van 32 MB buffer ofzo te denken.

[Sircuri]

je negeert mijn opmerking dat je op regel 32 niet weet hoeveel bytes er daadwerkelijk ingelezen zijn in je buffer array. Het klopt inderdaad dat je buffer 8kb groot is, maar met die constructie op regel 32 heb jij geen garantie dat er daadwerkelijk 8kb aan bytes zijn ingelezen. Er hoeft maar 1x iets fout te gaan in het inlezen en je hebt direct een corrupt bestand omdat je dan meer bytes wegschrijft naar je filehandle dan dat er gelezen zijn. je buffer bevat dan garbage uit de vorige lees actie.

[Janoz]

Je hebt geen garantie dat recv de volledige buffer vol schrijft. Stel je buffer is 16kb, maar recv leest maar 14kb in. Size wordt wel keurig verlaagd met 14kb, maar in de volgende regel schrijf je 14kb aan data en 2kb aan garbage welke nog in de buffer stond weg in je bestand.

Om het te testen moet je maar eens de hele lus lang de size wegschrijven. Waarschijnlijk zie je dan dat deze niet netjes met stappen van 16kb terugloopt.

[Sircuri]

En loopt deze wel netjes met 16kb terug per stap is dat puur dom geluk en geen enkele garantie dat het bij de volgende uitvoer van je programma ook zo gaat.

[Janoz]

Het zou best kunnen dat het toevoegen van de door mij voorgestelde code de boel niet ietsje vertraagd waardoor het probleem niet meer optreed. Andere manier zou kunnen zijn om een andere variabele bij te houden welke elke keer met buffersize wordt verminderd en vervolgens een dikke halt aanroept wanener deze en size ongelijk zijn.


Maar de oplossing is natuurlijk gewoon om het resultaat van recv in een apparte variabele te zetten en vervolgens enkel dat deel van de buffer wegschrijven ipv de hele buffer.

[High Quality king]

Ah ik begrijp het, sorry sircuri, het kwam niet in een keer helemaal binnen in mijn hoofd . Ik zal het aanpassen en de nieuwe code terug plaatsen in dit bericht.

code:

uint64_t sendBuffer(struct file *openFile, struct tcpHandle *connection) //check
{
    uint64_t size, result = 0;
    struct timeval start,end;
    gettimeofday(&start, NULL);
    
    sendSize(openFile->properties.st_size, connection);
    
    char buffer[TRANSFERBUFFER];
    
    while ((size = read(openFile->handle, buffer, TRANSFERBUFFER)) > 0)
    {
        result += send(connection->handle, buffer, size, 0);
        //printf("size : %lld result %lld\n", size, result);
        gettimeofday(&end, NULL);
        progressbar(result, openFile->properties.st_size, timeConversion(start, end) );
    }
    close(openFile->handle);
    
    if( result != (size = recvSize(connection)) )
    {
        printf("filesize %llu and sendsize %llu do not match",size, result);
    }
    
    return result;
}

uint64_t recvBuffer(struct file *openFile, struct tcpHandle *connection) // check
{
    struct timeval start,end;
    gettimeofday(&start, NULL);
    
    uint64_t sizeLeft = recvSize(connection), result = 0, size;
    openFile->properties.st_size = sizeLeft;
    
    char buffer[TRANSFERBUFFER];
    
    while( sizeLeft > TRANSFERBUFFER)
    {
        size = recv(connection->handle,buffer,TRANSFERBUFFER,0);
        result += write(openFile->handle, buffer, size);
        sizeLeft -= size;
        gettimeofday(&end, NULL);
        progressbar(result, openFile->properties.st_size, timeConversion(start, end) );
    }
    while( sizeLeft > 0)
    {
        size = recv(connection->handle, buffer, sizeLeft, 0); // result problem below on big files
        result += write(openFile->handle, buffer, size);
        sizeLeft -= size;
        gettimeofday(&end, NULL);
        progressbar(result, openFile->properties.st_size, timeConversion(start, end) );
    }
    close(openFile->handle);
    
    sendSize(result, connection); // confirm size
    
    return result;
}

Het werkt!!!! Super bedankt! Dubbele snelheid door het nu mogelijke grotere buffer. Enig probleem waar ik nu tegen aan loop is de max van char[max].

code:

char *buffer = malloc(TRANSFERBUFFER);
// bla bla code
free(buffer);

moet dat oplossen.

[ Voor 87% gewijzigd door High Quality king op 30-01-2014 11:50 ]

[farlane]

recv heeft iha ook de mogelijkheid om 0 of -1 te retouneren ipv minder dan het maximum.

[High Quality king]

farlane schreef op donderdag 30 januari 2014 @ 11:33:
recv heeft iha ook de mogelijkheid om 0 of -1 te retouneren ipv minder dan het maximum.

dat zou ik op kunnen lossen met?

code:

if( size > 0 ) {sizeLeft -= size;}
else 
{
//error
}

[Sircuri]

Beide while-loops zijn nog wel identiek. Je kan volstaan met een while(sizeLeft > 0)

[High Quality king]

Sircuri schreef op donderdag 30 januari 2014 @ 12:47:
Beide while-loops zijn nog wel identiek. Je kan volstaan met een while(sizeLeft > 0)

bijna identiek, nl:

code:

size = recv(connection->handle,buffer,TRANSFERBUFFER,0);
// verschil
size = recv(connection->handle,buffer,sizeleft,0);

Wel zou ik dit kunnen doen voor functie 2

code:

uint64_t recvBuffer(struct file *openFile, struct tcpHandle *connection) // check
{
    struct timeval start,end;
    gettimeofday(&start, NULL);
    
    uint64_t sizeLeft = recvSize(connection), result = 0, size;
    openFile->properties.st_size = sizeLeft;
    
    char buffer[TRANSFERBUFFER];
    
    while( sizeLeft > 0 )
    {
        if ( sizeLeft > TRANSFERBUFFER ) {size = recv(connection->handle,buffer,TRANSFERBUFFER,0);}
        else { size = recv(connection->handle, buffer, sizeLeft, 0);}
        result += write(openFile->handle, buffer, size);
        sizeLeft -= size;
        gettimeofday(&end, NULL);
        progressbar(result, openFile->properties.st_size, timeConversion(start, end) );
    }
    close(openFile->handle);
    
    sendSize(result, connection); // confirm size
    
    return result;
}

Of maak ik een denk fout?

[Sircuri]

Je kan altijd TRANSFERBUFFER gebruiken. Die parameter voor functie "recv" doet niets anders dan aangeven hoeveel bytes er maximaal passen in je buffer. Hoeveel bytes er daadwerkelijk gelezen zijn, komt immers terug via de return waarde van de functie "recv".

code:

    while( sizeLeft > 0 )
    {
        size = recv(connection->handle,buffer,TRANSFERBUFFER,0);
        result += write(openFile->handle, buffer, size);
        sizeLeft -= size;
        gettimeofday(&end, NULL);
        progressbar(result, openFile->properties.st_size, timeConversion(start, end) );
    }

[High Quality king]

Loop ik dan niet het risico dat als ik daarna een volgend bestand stuur, dat dat dan in het vorige bestand terecht komt?

[Soultaker]

Denk eraan dat send() en recv() (wat feitelijk varianten zijn op read() en write() met wat extra flags) ook -1 kunnen returnen wanneer er iets fout gaat. In het bijzonder kunnen ze onderbroken worden door een interrupt; in dat geval geldt dat errno == EINTR en kun je de call resumen. Bij write() kan het daardoor ook gebeuren dat de return value lager is dan size. In dat geval moet je de write() call ook herhalen.

[ Voor 3% gewijzigd door Soultaker op 30-01-2014 16:14 ]

[farlane]

farlane schreef op donderdag 30 januari 2014 @ 11:33:
recv heeft iha ook de mogelijkheid om 0 of -1 te retouneren ipv minder dan het maximum.

Shameless selfquote

[High Quality king]

is dit oplossing? @Farlane && Soultaker

code:

size_t sendall(int handle, void *buffer, size_t size, int flags)
{
    long outcome;
    size_t result = 0;
    while ( result < size)
    {
        outcome = send(handle,buffer,size,flags);
        result += outcome;
        if( outcome < 0 )
        {
            result -= outcome;
            // error handling
        }
    }
    return result;
}

size_t recvall(int handle, void *buffer, size_t size, int flags)
{
    long outcome;
    size_t result = 0;
    while ( result < size)
    {
        outcome = recv(handle,buffer,size,flags);
        result += outcome;
        if( outcome < 0 )
        {
            result -= outcome;
            // error handling
        }
    }
    return result;
}

[Sircuri]

Het lijkt erop dat je je eerst even moet verdiepen in de materie. Ik vind je voorgestelde code hierboven een beetje "twijfelachtig" alsof je niet helemaal snapt wat je eigen probleem is en ook niet hoe je het moet oplossen.
Ik zit nu even niet achter een pc (ipad) en om dan hele lappen code te knippen plakken is niet zo handig. Zal kijken of ik straks nog tijd heb.

[farlane]

Waarom controleer je niet eerst het resultaat van send en recv voordat je het bij je ontvangen/verzonden lengte optelt? Dit lijkt raar.

Beetje richting voor receive:

C:

int received = recv( ... );
if( received == - 1 )
{
    // Is fout, kijk naar errno of je het nog een keer mag proberen anders doe andere stuff
}
else
if( received == 0 )
{
    // Verbinding is bezig met een graceful shutdown, doe dat ook.
}
else
{
    // Handle blok ontvangen data
}

[ Voor 56% gewijzigd door farlane op 31-01-2014 11:21 ]

[High Quality king]

farlane schreef op vrijdag 31 januari 2014 @ 11:13:
Waarom controleer je niet eerst het resultaat van send en recv voordat je het bij je ontvangen/verzonden lengte optelt? Dit lijkt raar.

Beetje richting voor receive:

C:

int received = recv( ... );
if( received == - 1 )
{
    // Is fout, kijk naar errno of je het nog een keer mag proberen anders doe andere stuff
}
else
if( received == 0 )
{
    // Verbinding is bezig met een graceful shutdown, doe dat ook.
}
else
{
    // Handle blok ontvangen data
}

Dat is idd logischer. Met dit soort code van mij, merk je weer dat ik eigenlijk fotograaf ben :-). Dankjewel

code:

size_t sendall(int handle, void *buffer, size_t size, int flags)
{
    size_t result = 0;
    while ( result < size)
    {
        ssize_t outcome = send(handle,buffer,size,flags);
        if ( outcome > 0) { result += outcome; }
        else if ( outcome == 0 ){ /* reset connection */ }
        else if ( outcome == -1 ){ /* errorhanndling */ }
    }
    return result;
}

size_t recvall(int handle, void *buffer, size_t size, int flags)
{
    size_t result = 0;
    while ( result < size)
    {
        ssize_t outcome = recv(handle,buffer,size,flags);
        if ( outcome > 0) { result += outcome; }
        else if ( outcome == 0 ){ /* reset connection */ }
        else if ( outcome == -1 ){ /* errorhandling */ }
    }
    return result;
}

Ik heb er dit van gemaakt. Ik vond het logischer om eerst op succes te controleren, voordat ik alle uitzonderingen zou checken.

@sircuri het probleem in eerste instantie was dat de send en recv functie niet gegarandeerd hun data over zenden. Dat hebben we nu opgelost door de send en recv functie te vervangen door sendall en recvall. De functies hierboven zijn functies die wel overdracht garanderen en anders de error afhandelen.

Dit heb ik gedaan door het punt dat soultaker in detail maakt en waar farlane al eerder aan refereerde. Als send of recv -1 returned of een interupt stuurt, zou er corruptie optreden in het geschreven bestand (write). Dat wordt nu voorkomen.

Over niet begrijpen wat ik doe, ik hobby inderdaad wat aan, aan de andere kant is dit socket georienteerde kopieer programma tot nu toe sneller dan de finder kopieer functie (apple's variant op een kopieer actie in windows explorer).

[ Voor 54% gewijzigd door High Quality king op 31-01-2014 12:41 ]

[Soultaker]

High Quality King: je bent op de goede weg! Het is sowieso slim om deze functionaliteit in aparte functies onder te brengen zodat je het gedoe met return values checken kunt scheiden van de rest van je applicatie. Er zit nog wel een kleine fout in je logica: als recv() of send() een resultaat kleiner dan size teruggeeft, dan moet je niet de hele buffer opnieuw sturen/ontvangen, maar alleen het deel dat je nog niet hebt verstuurd/ontvangen.

Een positieve waarde van outcome betekent namelijk dat je precies zoveel bytes verstuurd/ontvangen hebt, en die moet je dus niet opnieuw sturen. (Waarbij outcome -1 met errno==EINTR een beetje een speciaal geval is; eigenlijk is dat een 0, maar 0 wordt al gebruikt om end-of-file mee aan te geven.)

In je code kun je dat simpel zo oplossen:

C:

        ssize_t outcome = send(handle,buffer+result,size-result,flags);

C:

        ssize_t outcome = recv(handle,buffer+result,size-result,flags);

[High Quality king]

duidelijk, maakt het nog uit dat ik een void pointer gebruik voor de buffer. Bij char weet hij natuurlijk dat hij 1 byte moet opschuiven, bij een 32 bit int 4 byte etc. Ik weet eigenlijk niet wat er gebeurd bij een void pointer, of neemt hij dan gewoon het pointer type van wat ik er heen stuur.

dus stel ik stuur er een int pointer naartoe, dan doet hij dat gedrag.
een char dat gedrag etc.

http://stackoverflow.com/...tic-for-void-pointer-in-c

Zij denken dat het niet mag, maar ik wil ook getallen verzenden middels mijn sendall en recvall.

[ Voor 16% gewijzigd door High Quality king op 31-01-2014 18:33 ]

[Soultaker]

Oh, dat was mijn fout: je moet officieel buffer inderdaad naar char* casten zodat je op byte-niveau addresseert. (GCC behandelt void* als char* wat betreft pointer arithmetic, maar dat is geen standaard gedrag.)

Normaalgesproken is p + i gelijk aan &p[i], wat dus betekent dat bij het adres dat in p opgeslagen wordt niet i bytes, maar i * sizeof(*p) bytes wordt opgeteld. Het type van p is bepalend; niet het type van het “echte” onderliggende object, want dat weet de compiler helemaal niet.

Kortom: de expressie (char*)buffer + result is correct.

[ Voor 3% gewijzigd door Soultaker op 31-01-2014 18:35 ]

[farlane]

Smaken verschillen, maar ik zou de functie waarschijnlijk een char* parameter voor de buffer laten gebruiken, en het (eventuele) casten aan de caller overlaten.

[High Quality king]

sendall & recvall hebben dezelfde parameters als send & recv, zodat ik deze functies als een soort upgrade van de originele functies kan gebruiken. Ik gebruik een void pointer niet alleen omdat dat hetzelfde is als de originele functie, maar ook omdat ik hem bijvoorbeeld gebruik om een uint64_t (!= char*) te versturen.

Dat de functie vervolgens per byte checkt of alles is aangekomen, maakt mij niet uit.

[ Voor 12% gewijzigd door High Quality king op 31-01-2014 20:15 ]

[farlane]

Een pointer naar een uint64_t* is ook te casten naar een char*, dus dat zal het probleem niet zijn . Het is meer semantiek denk ik, ik stel me de data die je verstuurd voor als een reeks bytes.
Ook is het zo dat de functies er wel van uit gaan dat het *een pointer* is naar de data die je wilt versturen, maar die void* maakt dat niet expliciet.

Just my 2cts

[Soultaker]

Ik ben het toch met HQking eens: void* is bedoeld als generiek pointer type om andere pointers van/naar te converteren (want op zichzelf is void niets). Vandaar dat alle standaardfuncties die iets met generieke blokken data doen, behalve read()/write() ook qsort(), malloc(), memcmp() et cetera, void* als argumenttype gebruiken.

Doordat void op zichzelf niets is, communiceert het gebruik van void* juist duidelijk dat het argument een willekeurig blok data kan zijn, in tegenstelling tot char*, wat een zero-terminated string (of tenminste een array van characters) impliceert.

Overigens geef impliciete conversie naar char* ook warnings, wat lelijk is.

[ Voor 33% gewijzigd door Soultaker op 31-01-2014 22:32 ]

[farlane]

Je hebt gelijk hoor, maar ik kan me eigenlijk niet voor de geest halen dat ik bij vergelijkbare functies voor void* heb gekozen. Meestal ( om de suggestie van een rits bytes te versterken, en die van een c-string te verzwakken ) kies ik voor uint8_t*.
Bij functionaliteit waar de gebruiker een eigen context zou kunnen meegeven zou ik dan weer wel kiezen voor een void*.

[ Voor 19% gewijzigd door farlane op 01-02-2014 13:49 ]