QTcpSocket question

tobias.hunger

codestein: Read up on network programming before continuing with this project of yours! Your receiver is just waiting for someone to bring down the whole machine. Network programming is tricky as it is easy to exploit any mistakes made remotely. So you need to know what you are doing or you will allow somebody to launch denial of service attacks against your machine.

The blocksize is much too high for most networks. I was thinking of a block size of maybe 64k or something! And you could get the same effect with much less code.

@
char * data[BLOCKSIZE];
quint64 filepos = 0;

while (filepos < file.size()) {
quint64 length = file.read(data, BLOCKSIZE);
filepos += length;
socket.write(data, length);
}
@

should do pretty much the same thing your code does.

There is no reason to seek() all the time. Reading etc. implicitly moves the position in the stream, so you are in the right position to read/write already.

@
if (tcpSocket->bytesAvailable() < totalfilesize) //Wait until everything is available
return;
@

So you send those 20 blueray disk rips in blocks, but at the receiver side you buffer all 20 of those in the network buffers before you start to write them out to disk? After making sure all the data of all files is already buffered in the receiver you then proceed to read the data in chunks. Why don't you just grab the whole thing in one go.

You should get the data from the OS as soon as possible. Read as much as possible whenever you get a readReady signal and save it out to files asap. As an optimization you should make sure that you write decently sized chunks of data to the file. It is painfully slow to append BLOCKSIZE times one byte to a file, and much faster to add 1 times BLOCKSIZE bytes.

lamprosg

I'm doing this small program for the sake of learning! Didn't know where to start so I went straight for the code, examples etc

bq. There is no reason to seek() all the time. Reading etc. implicitly moves the position in the stream, so you are in the right position to read/write already.

In the case of bytesAvailable, if I read 64k
@if (tcpSocket->bytesAvailable() < 64000)
return;
@

And then I want the other 64, I'll do it with the same code again and not like this, right?
@if (tcpSocket->bytesAvailable() < 128000)
return;
@

That was a major question I had.

Thanks for the replies!

tobias.hunger

bytesAvailable returns the number of bytes not yet read.

Actually I do not see any need to check tcpSocket->bytesAvailable(): In a slot triggered by readReady() you know there are bytes available. Just do something like this:

@
// const int BUFFERSIZE = 1024*1024;
// char * buffer[BUFFERSIZE];
// quint64 offset = 0;
// quint64 fileSize = 0;

void dataAvailable()
{
quint64 length = tcpSocket->read(buffer[offset], BUFFERSIZE - offset);
if (length + offset == BUFFERSIZE || length + fileSize == sizeOfFile[i]) {
file.write(buffer, BUFFERSIZE);
offset = 0;
fileSize += length;
// switch to next file if necessary!
} else {
offset += length;
}
}
@

goetz

There is no need to do the work in chunks of, say 64k. The operating system and Qt libs do some reasonable buffering for you, so there is no need to add another one and increase complexity of your code. Make it work correctly in the very first place, and, only if it is too slow, optimize it afterwards!

lamprosg

Greetings again!

Indeed after a number of debuggings I saw some descent buffering without coding it, smaller or arround 1 MB (many times much less). Probably best to put it in your code.

Still, I tried this code below, and I can't find any errors (downloading multiple files).
Seems to work for 1 file.. When the 1st file ends, it crashes where I put the asterisk. :O

@
qint64 savepos=0; //Position of bytes we're in
qint64 length=0; //Length of bytes read
int counter=0;
(bool) newfile=true;
qint64 buffer=1048576; // 1 MB
QFile *file;

        if (newfile == true)
        {
            file = new QFile&#40;filenames[counter]&#41;;         //Create the file
            file->open(QIODevice::WriteOnly);            //Open it
            newfile = false;
        }

        char *data = new char[buffer];
        length = tcpSocket->read(data, buffer);        //Read at max: 1MB
        file->write(data,length);
        savepos += length;
        ui->getprogressbar->setValue( (int) (savepos/filesizes[counter])*100);
        delete data;

      if (savepos == filesizes[counter])       //File size reached, saved and closing
      {
          file->close();
          delete file;
          counter++;
          savepos=0;                           //Reseting
          length=0;
          newfile = true;

          if (counter == numberofitems)
          {
              delete [] filenames;                                    //Setting the memory free
              delete [] filesizes;
              tcpSocket->disconnectFromHost();
          }
      }

@

goetz

Could you explain "crash" a bit more? Any kind of message on the console?

BTW: If you do data = new char[] you must call delete[] data afterwards, otherwise you have memory leak.

lamprosg

bq. If you do data = new char[] you must call delete[] data afterwards, otherwise you have memory leak.

You're totally right, how did I miss that?

Crash:
Signal name: SIGSEGV
Signal meaning: Segmentation fault

Points me to QAbstractSocketPrivate::resetSocketLayer

Now I'm thinking about it, it's possible with multiple files that savepos goes higher than the filesize after last reading of the first file (reads part of the 2d too) and savepos is never equal to filesize.. And goes all wrong from there

goetz

That sounds reasonable. We don't know what your "protocol" for the file transfer is. Maybe you should do it one by one, only sending a new file when the preceding has arrived completely.

On the other hand, if the equality never becomes true, you will write only one file that grows bigger and bigger. The segfault should not happen on the comparison line you indicated, but on the socket->read().

lamprosg

bq. The segfault should not happen on the comparison line you indicated, but on the socket->read().

That's where it crashed. The asterisk was the last line read, then went on the tcpSocket->read..

Anyway, I did it one by one. Seems it's working on the Simulator.. All I have to do is test it for real!
I hope I won't have any more surprises heh.

You've all been very helpful, and I learned few things.