ftpDataChannel RETR

JonB

@CMichDsl
This won't work, because inside the if you declare a new QTextStream stream which has nothing to do with the QTextStream stream outside the if.

You will want something more like [but see my EDIT below]:

    QFile *localfile = new QFile("file.csv");
    QTextStream *stream;
    if(localfile->open(QIODevice::WriteOnly | QIODevice::Text))
    {
        stream = new QTextStream(localfile);

        // Print all data retrieved from the server on the console.
        QObject::connect(&dataChannel, &FtpDataChannel::dataReceived,
                         [stream](const QByteArray &data) { // <-------------- stream captured to add data
            *stream << data.constData(); // <----------- instead of std::cout << data.constData();
        });

        // FTP terminate data transfer signal, whatever that is.
        QObject::connect(&dataChannel, &FtpDataChannel::dataFinished,
                         [stream, localFile]() {
            stream->close();
            stream->deleteLater();
            localFile->close();
            localFile->deleteLater();
        });
    }
    else
        localFile->deleteLater();

If you are in a class you might make QStream stream and QFile localfile class member variables, to avoid having a pointer and new/deleteLater(). The point is that both variables must be scoped/newed such that they persist until you receive the "data transfer finished" signal.

[EDIT] I only just saw this in yours:

ftpClient.submitEvent(command.cmd, command.args);
localfile.close();

This only works in your code if ftpClient.submitEvent() blocks until finished, I don't know if it does. But if it does you can simplify my suggestion, you won't need news or member variables. It will be closer to your original, but you must scope your QTextStream variable to persist, and be closed, like your file variable. If it does block, you can presumably just change your original to:

    QFile localfile("file.csv");
    if(localfile.open(QIODevice::WriteOnly | QIODevice::Text))
    {
         QTextStream stream(&localfile);

        // Print all data retrieved from the server on the console.
        QObject::connect(&dataChannel, &FtpDataChannel::dataReceived,
                         [&stream](const QByteArray &data) { // <-------------- stream captured to add data
            stream << data.constData(); // <----------- instead of std::cout << data.constData();
        });

        ... 
        ftpClient.submitEvent(command.cmd, command.args);
        stream.close();
        localfile.close();
    }

Bonnie

@CMichDsl Why are you using QTextStream to write QByteArray? What if the file is binary?
You can simply call

qint64 QIODevice::write(const QByteArray &byteArray)

QFile is also a QIODevice.

JonB

@Bonnie
I assume that user intends QFile localfile("file.csv"); to be transferred as a text file, because that's what a CSV file is. And his FTP "RETR" command has been told to transfer in text mode (it said Opening ASCII mode).

If you use QIODevice/QFile::write(const QByteArray &byteArray) directly, does any buffering go on, or does this call write(2) directly?

Bonnie

@JonB I think QFile has its own buffering.
And I don't think it is right, in a file transfering, to convert QByteArray to QString, and finally to QByteArray, again.
That's what QTextStream do when write buffer to io device, it calls QIODevice::write(const QByteArray &byteArray).

CMichDsl

Ok thanks for the advice, I didn't know

I try with QFile::write but I don't know where to open the file (should be in the QObject::connect? And append data?)
If I open the file outside the callback, the file is opened, but when I write data it gives me the error "device not open"

    QFile localfile("localfile.csv");

    if(!localfile.open(QIODevice::WriteOnly | QIODevice::Text))
    {
        qDebug() << "File not opened";
        return 1;
    }
    else
    {
        qDebug() << "File opened";
    }
    
    QObject::connect(&dataChannel, &FtpDataChannel::dataReceived,
            [&localfile](const QByteArray &data) {
        localfile.write(data);
        //std::cout << data.constData();
    });

    ftpClient.submitEvent("cmd.RETR", "file.csv");
    localfile.close();

File opened
Posting command "cmd.RETR" "file.csv"
Reply "reply.150" "Opening ASCII mode data connection for file.csv (5664 bytes)."
QIODevice::write (QFile, "localfile.csv"): device not open
QIODevice::write (QFile, "localfile.csv"): device not open
QIODevice::write (QFile, "localfile.csv"): device not open
Reply "reply.226" "Transfer complete."

jsulm

@CMichDsl said in ftpDataChannel RETR:

QFile localfile("localfile.csv");

This is local to the function where it is located, right? It goes out of scope and the file is closed.
Just add it as class member to your class. Or open it in the lambda.

CMichDsl

In fact FtpDataChannel::dataReceived is called asynchronously, so file is opened, and directly closed by localfile.close();
If I remove localfile.close(); content is write in file

>> open it in the lambda.

As lambda is called many times (file is downloaded in multiple parts) I have to clear file, and open it in Append mode, close it, reopen it... not very usefull, there should be a etter way

    // Clear the file
    if(!localfile.open(QIODevice::WriteOnly | QIODevice::Text))
    {
        qDebug() << "File not opened";
        return 1;
    }
    else
    {
        localfile.close();
        qDebug() << "File cleared";
    }

    // Append all data retrieved from the server in the file.
    QObject::connect(&dataChannel, &FtpDataChannel::dataReceived,
                     [&localfile](const QByteArray &data) {
        if(!localfile.open(QIODevice::Append | QIODevice::Text))
        {
            return 1;
        }
        else
        {
            localfile.write(data);
            localfile.close();
        }
    });

jsulm

@CMichDsl I don't understand the problem: do you want to append to the file? If so then just open it once in append mode.

JonB

@CMichDsl
As @jsulm has said, and as I showed in the code I gave you, open the file before you try to receive stuff (i.e. just before your RETR call) and close it when you get whatever "finished" signal. I said you must keep localFile in scope, e.g. before your RETR call if it's blocking, as a class member variable if you have an instance which lives throughout the transfer, or via a new if necessary.

What you have done in your latest code is to re-open for append, write and close the file against each dataReceived signal. While this will work, it is not what would be expected/could be slow if you are receiving many dataReceived signals, which I imagine you will be as it's a file transfer.

CMichDsl

No I just want to get file
so... open, write then close the file

I'm moving forward, but I'm not sure it's the right way

I can open the file at the beginning of the class
start the connection and run commands (cmd.USER, cmd.PORT, cmd.CWD and cmd.RETR)
cmd.RETR write the contents

But "localfile" is not closed (I cannot close it otherwise it is "not opened")

jsulm

@CMichDsl said in ftpDataChannel RETR:

so... open, write then close the file

Since you get the data in several parts you have to open the file in append mode and close it when you got all the data as @JonB explained...

JonB

@CMichDsl said in ftpDataChannel RETR:

But "localfile" is not closed (I cannot close it otherwise it is "not opened")

There must be some signal/event when the RETR has completed/finished, or even errorred, from whatever package/functions you are using. In my example I referred to it as signal &FtpDataChannel::dataFinished cf. the &FtpDataChannel::dataReceived you use; you need to find it from whatever docs? That is when you need to close the file.

CMichDsl

@JonB
>> and close it when you get whatever "finished" signal

I understood but didn't know if Icounld rely on reply code (226)

Reply "reply.150" "Opening ASCII mode data connection for pp.csv (5664 bytes)."
Reply "reply.226" "Transfer complete."

>> s signal &FtpDataChannel::dataFinished

There's no dataFinished in the example but you put me on the way
I have to define it by myself in ftpdatachannel.cpp

----------------- ftpdatachannel.h

signals:

    // The FTP server has sent some data.
    void dataReceived(const QByteArray &data);

private:
    QTcpServer m_server;


----------------- ftpdatachannel.cpp

FtpDataChannel::FtpDataChannel(QObject *parent) : QObject(parent)
{
    connect(&m_server, &QTcpServer::newConnection, this, [this]() {
        m_socket.reset(m_server.nextPendingConnection());
        connect(m_socket.data(), &QTcpSocket::readyRead, [this]() {
            emit dataReceived(m_socket->readAll());
        });
    });
}

i'll look at docs! It's going to be less easy than I thought
Thanks

JonB

@CMichDsl said in ftpDataChannel RETR:

I understood but didn't know if Icounld rely on reply code (226)

You'll have to rely on something, else you'd never know when the data transfer is finished if the server doesn't tell you! Which all servers must do in an FTP scenario.

As you say, you'll have to consult your docs, or even the FTP RFC (which is what I used). I can tell you now that 226 won't be enough, at least for anything robust. From my recollection, TCP messages are banded by their hundreds value. 100 stuff is something like "informational, all is well carry on", like your initial 150. 200 stuff is something like "this is positive", I can't say if 226 is the only one to expect from RETR. But then we get to something like 300+ which I think is errors. You're going to need to deal with those too, and (probably) close and delete your local file if you get one. FTP transfers/connections do definitely fail in the real world :)

And, sadly, FTP servers vary in their behaviour & responses, so you're going to find it very hard to test all possibilities unless you only support one specific environment. Read the FTP docs and use the hundreds banding ranges of the responses to (try to) handle cases you won't necessarily meet.

CMichDsl

Sadly, yes
I think I'll fall back on QFtp :-)

I looked at this code, i will not re-inventing the wheel
https://github.com/qt/qtftp/blob/master/src/qftp/qftp.cpp

It's a shame to have deleted it from Qt (?)

JonB

@CMichDsl
Indeed I would look at that if you are going to have it all yourself. However, isn't that Qt4 only? I don't think it was ported. You are supposed to use QNetworkAccessManager now, but I don't think it supports your RETR, but can do GET.

https://stackoverflow.com/questions/23603494/porting-qftp-in-qt4-to-qt5-using-qnetworkaccessmanager
https://forum.qt.io/topic/26604/replacement-for-qftp

It became unsupported. You are not the first to bemoan its loss.