API review:

* Account::createFileTransfer: \param fileName should probably be called suggestedFileName to emphasize that this is not how you provide the actual data

* Account::createFileTransfer: document how you send "unspecified hash type" (from the code, the answer appears to be ((FileHashType) -1)

* We also need convenience API which will look at RequestableChannelClasses and tell you which properties you're allowed (or required!) to set

* There seems to be no way to refrain from including a filename, content-type, size, description and date in the request - we should probably specmeet about this, and decide whether CMs are required to allow all of these for FT channels even if they won't do anything useful with them?

Code review as I went past (I haven't done a detailed review):

* Sender and Receiver should probably print a line to stderr every time they see progress, to illustrate how you'd hook up a progress bar

* Receiver::onFileTransferChannelReady: I know it's only an example, but happily overwrite existing files is bad; could you either use an equivalent of O_EXCL, or do something like saving the file as ("TelepathyQt4ReceiverExample_" + fileName().replace("/", "_"))?

Branch updated and ready for review

(Please add the patch keyword when this is ready for review again.)

> - * \headerfile TelepathyQt4/account.h> <TelepathyQt4/Account>
> + * \headerfile <TelepathyQt4/account.h> <TelepathyQt4/Account>

This should have been in a trivia branch; it has nothing to do with what you're mainly working on, and can be merged even if the rest is delayed or rejected.

In the Account:

> +    QFileInfo fileInfo(properties.suggestedFileName());
> +    request.insert(QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER ".Filename"),
> +                   fileInfo.fileName());

I hope this is the Qt idiom for basename(3), rather than something that does I/O? What we want here is basename(3) or the Qt equivalent.

In the channel factory:

> -        return ChannelPtr(dynamic_cast<Channel*>(
> -                    FileTransferChannel::create(connection,
> -                        channelPath, immutableProperties).data()));
> +        if (immutableProperties.contains(QLatin1String(
> +                        TELEPATHY_INTERFACE_CHANNEL ".Requested"))) {
...
> +        } else {
> +            return ChannelPtr(dynamic_cast<Channel*>(
> +                        FileTransferChannel::create(connection,
> +                            channelPath, immutableProperties).data()));
> +        }

Is this "else" clause ever going to be useful, given that the abstract FT channel class is basically useless? I'm not sure what we could do better, though - perhaps warn that the CM is being stupid, and perhaps return a Channel base-class instance?

In the FileTransferChannel:

> +/**
> + * Return the name of the file on the sender's side. This is therefore given as
> + * a suggested filename for the receiver. This cannot change once the channel
> + * has been created.

s/therefore//

> +/**
> + * The size of the file. This cannot change once the channel has been
> + * created.
> + *
> + * This method requires FileTransferChannel::FeatureCore to be enabled.
> + *
> + * \return The size of the file.
> + */

Say explicitly here that the size is not guaranteed to be exactly right for incoming files! Give the example of a protocol where sizes are internally passed around as KB and sizes get rounded up, for instance.

Also explain that this can be UINT64_MAX (or whatever that's called in Qt) for unknown sizes.

> +/**
> + * Return the hash of the contents of the file transfer, of type described in
> + * the value of the contentHashType().
> + *
> + * Its value MUST correspond to the appropriate type of the contentHashType().
> + * If the contentHashType() is set to %FileHashTypeNone,
> + * then this value should be ignored.
> + *
> + * This method requires FileTransferChannel::FeatureCore to be enabled.
> + *
> + * \return Hash of the contents of the file transfer.
> + * \sa contentHashType()
> + */
> +QString FileTransferChannel::contentHash() const
> +{
> +    if (!isReady(FeatureCore)) {
> +        warning() << "FileTransferChannel::FeatureCore must be ready before "
> +            "calling contentHash";
> +    }
> +
> +    return mPriv->contentHash;

Please return "" if the hash type is NONE, which means you can change the second paragraph to: "If the contentHashType() is %FileHashTypeNone, then the hash is always an empty string".

> +/**
> + * Return the offset in bytes from where the file should be sent.
> + *
> + * This method requires FileTransferChannel::FeatureCore to be enabled.
> + *
> + * \return The offset in bytes from where the file should be sent.
> + */
> +qulonglong FileTransferChannel::initialOffset() const
> +{
> +    if (!isReady(FeatureCore)) {
> +        warning() << "FileTransferChannel::FeatureCore must be ready before "
> +            "calling initialOffset";
> +    }
> +
> +    return mPriv->initialOffset;
> +}

s/from where/from which/

s/should/will/

> +    PendingVariant *pv = new PendingVariant(
> +            fileTransferInterface(BypassInterfaceCheck)->AcceptFile(SocketAddressTypeIPv4,
> +                SocketAccessControlLocalhost, QDBusVariant(QVariant(QString())),
> +                offset),
> +            this);

"Localhost" access control means that the CM will accept any connection from any process on the local host, and give them your file (local privilege escalation). We should do Port access control if the CM supports it, which is done like this:

* create a TCP socket
* bind() to an unspecified port on localhost (127.0.0.1 port 0) - this will make the kernel allocate a high port for you
* getsockname() to find out what port we got (suppose it's 12345)
* pass the address and port we were given in the access control parameter
* the CM will only allow connections from that port, closing the security hole

(I don't know how you do those things in Qt-land...)

This is not necessarily a merge blocker, but it's a release blocker.

If the CM is sufficiently deficient that it can't do Port access control, I think it's acceptable to give up and rely on Localhost, though.

Please also mention in the docs that only the primary handler of a FT channel may call acceptFile.

> +void IncomingFileTransferChannel::onAcceptFileFinished(PendingOperation *op)
> +{
> +    if (op->isError()) {
> +        warning() << "Error accepting file transfer " <<
> +            op->errorName() << ":" << op->errorMessage();
> +        return;
> +    }
> +
> +    PendingVariant *pv = qobject_cast<PendingVariant *>(op);
> +    mPriv->addr = qdbus_cast<SocketAddressIPv4>(pv->result());
> +    debug().nospace() << "Got address " << mPriv->addr.address <<
> +        ":" << mPriv->addr.port;

Please detect failures of the qdbus_cast (CM gave us the wrong type back).

If AcceptFile() fails we should probably invalidate the channel.

> +    debug() << "Connected to host!";

Stylistic: no! random! exclamation! marks! please! Why are you so surprised that we connected to the host? :-)

> +void IncomingFileTransferChannel::doTransfer()
> +{
> +    QByteArray data;
> +    while (mPriv->socket->bytesAvailable()) {
> +        data = mPriv->socket->readAll();
> +        mPriv->output->write(data); // never fails

O rly? Does Qt have an unlimited outgoing socket buffer in userland, or are you just assuming that the kernel buffer will never fill up?

I think we discussed the idea of having IFTC automatically drop n initial bytes if the initialOffset is smaller than we asked for (in particular, if the sender starts again at the beginning)?

IFTC should certainly fail if the initialOffset is *larger* than we asked for, since that leaves a gap :-P (this can only happen if the CM or sender is being stupid)

> +/**
> + * Provide the file for an outgoing file transfer which has been offered.
> + * The state will change to %FileTransferStateOpen as soon as the transfer
> + * starts.
> + * The given input device should not be destroyed until the state()
> + * changes to %FileTransferStateCompleted or %FileTransferStateCancelled.
> + * If input is a sequential device QIODevice::isSequential(), it should be
> + * closed when no more data is available, so we know when to stop reading.
> + *
> + * This method requires FileTransferChannel::FeatureCore to be enabled.
> + *
> + * \param input A QIODevice object where the data will be read from.
> + * \return A PendingOperation object which will emit PendingOperation::finished
> + *         when the call has finished.
> + * \sa stateChanged(), state(), stateReason()
> + */

Again, explicitly say that only the primary handler of the FT channel may call this.

Again, some access control would be good.

> +    // in case of sequential devices, we should read everything from it and
> +    // write to the socket. Let's not do this for non-sequential devices as we
> +    // don't want to read a whole file into memory.
> +    if (isConnected() && mPriv->input->isSequential()) {
> +        QByteArray data;
> +        data = mPriv->input->readAll();
> +        mPriv->socket->write(data); // never fails
> +    }

How does this work? I suspect that either this is unnecessary because we're just going to stream until EOF anyway (in which case it's unnecessary for sequential files too), or it's necessary (in which case we have a bug in the non-sequential (pipe) case)?

> +    // read 16k each time, as input can be a QFile, we don't want to block
> +    // reading the whole file
> +    char buffer[16 * 1024];

Symbolic constant (const int or #define), please! FT_BLOCK_SIZE would be a suitable name.

Regarding the Receiver example, it would be good to have a command-line option (or something) that wants to skip the first kilobyte of the file, as a demo of how offsets work.

It would also be good to have a command-line option that writes the file to stdout instead of a file, to test the non-seekable code path.

While sending, please have a command-line option that reads from stdin (again to test the non-seekable code path), and please test it with a nonzero offset.

(In reply to comment #3)
> (Please add the patch keyword when this is ready for review again.)
> 
> > - * \headerfile TelepathyQt4/account.h> <TelepathyQt4/Account>
> > + * \headerfile <TelepathyQt4/account.h> <TelepathyQt4/Account>
> 
> This should have been in a trivia branch; it has nothing to do with what you're
> mainly working on, and can be merged even if the rest is delayed or rejected.
> 
> In the Account:
> 
My fault, probably a git commit -a :S. I will check if this is in a separate patch and if so I will rebase this

> > +    QFileInfo fileInfo(properties.suggestedFileName());
> > +    request.insert(QLatin1String(TELEPATHY_INTERFACE_CHANNEL_TYPE_FILE_TRANSFER ".Filename"),
> > +                   fileInfo.fileName());
> 
> I hope this is the Qt idiom for basename(3), rather than something that does
> I/O? What we want here is basename(3) or the Qt equivalent.
> 

 QFileInfo fi("/tmp/archive.tar.gz");
 QString name = fi.fileName();                // name = "archive.tar.gz"

 QFileInfo fi("/tmp/archive.tar.gz");
 QString base = fi.baseName();  // base = "archive"

In this case we want the file name without the path

> In the channel factory:
> 
> > -        return ChannelPtr(dynamic_cast<Channel*>(
> > -                    FileTransferChannel::create(connection,
> > -                        channelPath, immutableProperties).data()));
> > +        if (immutableProperties.contains(QLatin1String(
> > +                        TELEPATHY_INTERFACE_CHANNEL ".Requested"))) {
> ...
> > +        } else {
> > +            return ChannelPtr(dynamic_cast<Channel*>(
> > +                        FileTransferChannel::create(connection,
> > +                            channelPath, immutableProperties).data()));
> > +        }
> 
> Is this "else" clause ever going to be useful, given that the abstract FT
> channel class is basically useless? I'm not sure what we could do better,
> though - perhaps warn that the CM is being stupid, and perhaps return a Channel
> base-class instance?
> 
I prefer to return a base FTChannel than a bare Channel class. The user will have access to all Channel class methods anyway, as FTChannel inherits Channel.

The most common way to reach this code path is when the user calls ChannelFactory::create. Added a warning.

> In the FileTransferChannel:
> 
> > +/**
> > + * Return the name of the file on the sender's side. This is therefore given as
> > + * a suggested filename for the receiver. This cannot change once the channel
> > + * has been created.
> 
> s/therefore//
Done

> > +/**
> > + * The size of the file. This cannot change once the channel has been
> > + * created.
> > + *
> > + * This method requires FileTransferChannel::FeatureCore to be enabled.
> > + *
> > + * \return The size of the file.
> > + */
> 
> Say explicitly here that the size is not guaranteed to be exactly right for
> incoming files! Give the example of a protocol where sizes are internally
> passed around as KB and sizes get rounded up, for instance.
> 
> Also explain that this can be UINT64_MAX (or whatever that's called in Qt) for
> unknown sizes.
Done
 
> > +/**
> > + * Return the hash of the contents of the file transfer, of type described in
> > + * the value of the contentHashType().
> > + *
> > + * Its value MUST correspond to the appropriate type of the contentHashType().
> > + * If the contentHashType() is set to %FileHashTypeNone,
> > + * then this value should be ignored.
> > + *
> > + * This method requires FileTransferChannel::FeatureCore to be enabled.
> > + *
> > + * \return Hash of the contents of the file transfer.
> > + * \sa contentHashType()
> > + */
> > +QString FileTransferChannel::contentHash() const
> > +{
> > +    if (!isReady(FeatureCore)) {
> > +        warning() << "FileTransferChannel::FeatureCore must be ready before "
> > +            "calling contentHash";
> > +    }
> > +
> > +    return mPriv->contentHash;
> 
> Please return "" if the hash type is NONE, which means you can change the
> second paragraph to: "If the contentHashType() is %FileHashTypeNone, then the
> hash is always an empty string".
Done

> > +/**
> > + * Return the offset in bytes from where the file should be sent.
> > + *
> > + * This method requires FileTransferChannel::FeatureCore to be enabled.
> > + *
> > + * \return The offset in bytes from where the file should be sent.
> > + */
> > +qulonglong FileTransferChannel::initialOffset() const
> > +{
> > +    if (!isReady(FeatureCore)) {
> > +        warning() << "FileTransferChannel::FeatureCore must be ready before "
> > +            "calling initialOffset";
> > +    }
> > +
> > +    return mPriv->initialOffset;
> > +}
> 
> s/from where/from which/
> 
> s/should/will/
Done
 
> > +    PendingVariant *pv = new PendingVariant(
> > +            fileTransferInterface(BypassInterfaceCheck)->AcceptFile(SocketAddressTypeIPv4,
> > +                SocketAccessControlLocalhost, QDBusVariant(QVariant(QString())),
> > +                offset),
> > +            this);
> 
> "Localhost" access control means that the CM will accept any connection from
> any process on the local host, and give them your file (local privilege
> escalation). We should do Port access control if the CM supports it, which is
> done like this:
> 
> * create a TCP socket
> * bind() to an unspecified port on localhost (127.0.0.1 port 0) - this will
> make the kernel allocate a high port for you
> * getsockname() to find out what port we got (suppose it's 12345)
> * pass the address and port we were given in the access control parameter
> * the CM will only allow connections from that port, closing the security hole
> 
> (I don't know how you do those things in Qt-land...)
> 
> This is not necessarily a merge blocker, but it's a release blocker.
> 
> If the CM is sufficiently deficient that it can't do Port access control, I
> think it's acceptable to give up and rely on Localhost, though.
> 
Working on this

> Please also mention in the docs that only the primary handler of a FT channel
> may call acceptFile.
> 
Done

> > +void IncomingFileTransferChannel::onAcceptFileFinished(PendingOperation *op)
> > +{
> > +    if (op->isError()) {
> > +        warning() << "Error accepting file transfer " <<
> > +            op->errorName() << ":" << op->errorMessage();
> > +        return;
> > +    }
> > +
> > +    PendingVariant *pv = qobject_cast<PendingVariant *>(op);
> > +    mPriv->addr = qdbus_cast<SocketAddressIPv4>(pv->result());
> > +    debug().nospace() << "Got address " << mPriv->addr.address <<
> > +        ":" << mPriv->addr.port;
> 
> Please detect failures of the qdbus_cast (CM gave us the wrong type back).
Do we really need that? We don't make checks in any other place, and actually the channel will do nothing if this happens, mPriv->addr.address.isNull() will be true. If we are going to handle this what should we do? Invalidate the channel?

> If AcceptFile() fails we should probably invalidate the channel.
Done, same for ProvideFile

> > +    debug() << "Connected to host!";
> 
> Stylistic: no! random! exclamation! marks! please! Why are you so surprised
> that we connected to the host? :-)
hehe, done, btw that is being really pedantic :P

> > +void IncomingFileTransferChannel::doTransfer()
> > +{
> > +    QByteArray data;
> > +    while (mPriv->socket->bytesAvailable()) {
> > +        data = mPriv->socket->readAll();
> > +        mPriv->output->write(data); // never fails
> 
> O rly? Does Qt have an unlimited outgoing socket buffer in userland, or are you
> just assuming that the kernel buffer will never fill up?
>
afaik write in Qt will never fail, unless the socket closes or something, so yeah they have a buffer internally. bytesWritten will be called when the buffer is actually written to the output device. I will double check this

> I think we discussed the idea of having IFTC automatically drop n initial bytes
> if the initialOffset is smaller than we asked for (in particular, if the sender
> starts again at the beginning)?
> 
> IFTC should certainly fail if the initialOffset is *larger* than we asked for,
> since that leaves a gap :-P (this can only happen if the CM or sender is being
> stupid)
I don't remember we discussing this, we certainly discussed about OFTC skipping the first N bytes until initialOffset, which is actually implemented.
Anyway I will work on this. Btw fail in what sense? Close the FTChannel?
 
> > +/**
> > + * Provide the file for an outgoing file transfer which has been offered.
> > + * The state will change to %FileTransferStateOpen as soon as the transfer
> > + * starts.
> > + * The given input device should not be destroyed until the state()
> > + * changes to %FileTransferStateCompleted or %FileTransferStateCancelled.
> > + * If input is a sequential device QIODevice::isSequential(), it should be
> > + * closed when no more data is available, so we know when to stop reading.
> > + *
> > + * This method requires FileTransferChannel::FeatureCore to be enabled.
> > + *
> > + * \param input A QIODevice object where the data will be read from.
> > + * \return A PendingOperation object which will emit PendingOperation::finished
> > + *         when the call has finished.
> > + * \sa stateChanged(), state(), stateReason()
> > + */
> 
> Again, explicitly say that only the primary handler of the FT channel may call
> this.
Done
 
> Again, some access control would be good.
Working on this

> > +    // in case of sequential devices, we should read everything from it and
> > +    // write to the socket. Let's not do this for non-sequential devices as we
> > +    // don't want to read a whole file into memory.
> > +    if (isConnected() && mPriv->input->isSequential()) {
> > +        QByteArray data;
> > +        data = mPriv->input->readAll();
> > +        mPriv->socket->write(data); // never fails
> > +    }
> 
> How does this work? I suspect that either this is unnecessary because we're
> just going to stream until EOF anyway (in which case it's unnecessary for
> sequential files too), or it's necessary (in which case we have a bug in the
> non-sequential (pipe) case)?
There is no way to know EOF is reached for a sequential device. Imagine the following scenario.
 - The input is a QTcpSocket or stdin and the transfer did not finished (network lag or user stopped typing) but "EOF" is reached.

For sequential devices the only way to know it finished is either connecting to aboutToClose (the user closed the input device) or using the size (which we can't). Reading from a sequential device will always return 0 if there is nothing to read. For stdin the app could ask the user to type EOF for example and close the input device, and we would know the transfer finished (aboutToClose would be emitted).
So you got the idea.

> > +    // read 16k each time, as input can be a QFile, we don't want to block
> > +    // reading the whole file
> > +    char buffer[16 * 1024];
> 
> Symbolic constant (const int or #define), please! FT_BLOCK_SIZE would be a
> suitable name.
Done

> 
> Regarding the Receiver example, it would be good to have a command-line option
> (or something) that wants to skip the first kilobyte of the file, as a demo of
> how offsets work.
> 
> It would also be good to have a command-line option that writes the file to
> stdout instead of a file, to test the non-seekable code path.
> 
> While sending, please have a command-line option that reads from stdin (again
> to test the non-seekable code path), and please test it with a nonzero offset.
Working on this 

> > > +    PendingVariant *pv = new PendingVariant(
> > > +            fileTransferInterface(BypassInterfaceCheck)->AcceptFile(SocketAddressTypeIPv4,
> > > +                SocketAccessControlLocalhost, QDBusVariant(QVariant(QString())),
> > > +                offset),
> > > +            this);
> > 
> > "Localhost" access control means that the CM will accept any connection from
> > any process on the local host, and give them your file (local privilege
> > escalation). We should do Port access control if the CM supports it, which is
> > done like this:
> > 
> > * create a TCP socket
> > * bind() to an unspecified port on localhost (127.0.0.1 port 0) - this will
> > make the kernel allocate a high port for you
> > * getsockname() to find out what port we got (suppose it's 12345)
> > * pass the address and port we were given in the access control parameter
> > * the CM will only allow connections from that port, closing the security hole
> > 
> > (I don't know how you do those things in Qt-land...)
> > 
> > This is not necessarily a merge blocker, but it's a release blocker.
> > 
> > If the CM is sufficiently deficient that it can't do Port access control, I
> > think it's acceptable to give up and rely on Localhost, though.
> > 
Done, but not tested as latest gabble does not support Port access control for FT.

> > I think we discussed the idea of having IFTC automatically drop n initial bytes
> > if the initialOffset is smaller than we asked for (in particular, if the sender
> > starts again at the beginning)?
> > 
> > IFTC should certainly fail if the initialOffset is *larger* than we asked for,
> > since that leaves a gap :-P (this can only happen if the CM or sender is being
> > stupid)
> I don't remember we discussing this, we certainly discussed about OFTC skipping
> the first N bytes until initialOffset, which is actually implemented.
> Anyway I will work on this. Btw fail in what sense? Close the FTChannel?
Done

> > Regarding the Receiver example, it would be good to have a command-line option
> > (or something) that wants to skip the first kilobyte of the file, as a demo of
> > how offsets work.
Done

> > It would also be good to have a command-line option that writes the file to
> > stdout instead of a file, to test the non-seekable code path.
This is actually not needed, as there is not seek involved in writing to the output device while receiving a file, you just write and close when the transfer finishes.

> > While sending, please have a command-line option that reads from stdin (again
> > to test the non-seekable code path), and please test it with a nonzero offset.
Tested with non-zero offset, using the receiver example requesting a file with offset != 0. Reading from stdin in Qt is a bit awkward, as there is no QIODevice which is able to read from stdin, I will try using QFile and open(stdin) but I am not sure if that will work. If this last item is non-blocker, the branch is ok for review.

(In reply to comment #4)
> > I hope this is the Qt idiom for basename(3), rather than something that does
> > I/O? What we want here is basename(3) or the Qt equivalent.
> 
>  QFileInfo fi("/tmp/archive.tar.gz");
>  QString name = fi.fileName();                // name = "archive.tar.gz"

Right, but the point I was trying to make (which I perhaps didn't express very well) is the difference between basename(3) and stat(2). Does QFileInfo look at the filesystem, or is it just an abstract API for dealing with filenames?

What we want is an equivalent of basename(3) or Python's os.path.basename(), which don't look at the filesystem at all, but instead have the semantics of "if we imagined that this was a file path, what would the filename be?"

From the docs, it appears that QFileInfo is both :-( but fileName() seems to work OK without looking at the filesystem.

Perhaps we shouldn't do the parsing at all, and should just document that API users are meant to pass in a bare filename, mentioning QFileInfo::fileName in the docs? After all, to us, the CM and the protocol it's just descriptive text really.

> > > +    PendingVariant *pv = qobject_cast<PendingVariant *>(op);
> > > +    mPriv->addr = qdbus_cast<SocketAddressIPv4>(pv->result());
> > > +    debug().nospace() << "Got address " << mPriv->addr.address <<
> > > +        ":" << mPriv->addr.port;
> > 
> > Please detect failures of the qdbus_cast (CM gave us the wrong type back).
> Do we really need that? We don't make checks in any other place, and actually
> the channel will do nothing if this happens, mPriv->addr.address.isNull() will
> be true. If we are going to handle this what should we do? Invalidate the
> channel?

Yeah, I think if the CM gives us stupid input, we should respond by closing and invalidating the channel (that's the same thing that telepathy-farsight does on fatal streaming errors).

> > I think we discussed the idea of having IFTC automatically drop n initial bytes
> > if the initialOffset is smaller than we asked for (in particular, if the sender
> > starts again at the beginning)?
> > 
> > IFTC should certainly fail if the initialOffset is *larger* than we asked for,
> > since that leaves a gap :-P (this can only happen if the CM or sender is being
> > stupid)
> I don't remember we discussing this, we certainly discussed about OFTC skipping
> the first N bytes until initialOffset, which is actually implemented.
> Anyway I will work on this.

To be clear: this is to handle the case where the API user says "please start from offset 1234", and the CM replies "starting from offset 1024", e.g. because the protocol internally works in 1K blocks. In that case I think we should skip past those 210 extra bytes before starting to write to the API user's socket.

> Btw fail in what sense? Close the FTChannel?

Yeah - nothing else would be safe (we're missing a piece of the file!). I think we have a TpQt4-specific error string somewhere already that means "CM is being stupid", equivalent to TP_DBUS_ERROR_INCONSISTENT in telepathy-glib? If we say "please start from offset 1234" and the CM replies "starting from offset 2048", we should invalidate the Channel with that error, and close it.

> > > +    // in case of sequential devices, we should read everything from it and
> > > +    // write to the socket. Let's not do this for non-sequential devices as we
> > > +    // don't want to read a whole file into memory.
> > > +    if (isConnected() && mPriv->input->isSequential()) {
> > > +        QByteArray data;
> > > +        data = mPriv->input->readAll();
> > > +        mPriv->socket->write(data); // never fails
> > > +    }
> > 
> > How does this work? I suspect that either this is unnecessary because we're
> > just going to stream until EOF anyway (in which case it's unnecessary for
> > sequential files too), or it's necessary (in which case we have a bug in the
> > non-sequential (pipe) case)?
> There is no way to know EOF is reached for a sequential device. Imagine the
> following scenario.
>  - The input is a QTcpSocket or stdin and the transfer did not finished
> (network lag or user stopped typing) but "EOF" is reached.
> 
> For sequential devices the only way to know it finished is either connecting to
> aboutToClose (the user closed the input device) or using the size (which we
> can't). Reading from a sequential device will always return 0 if there is
> nothing to read. For stdin the app could ask the user to type EOF for example
> and close the input device, and we would know the transfer finished
> (aboutToClose would be emitted).
> So you got the idea.

Hmm. This still raises red flags for me... if we always read as much as is available, surely aboutToClose will never be emitted with data still in the queue?

How this would work in a standard Unix-file-descriptor situation with non-blocking sockets is:

* if select() says the fd has nothing to read, go back to the main loop (done implicitly in GLib/Qt, in practice)
* if read() returns -1 with errno==EWOULDBLOCK or errno==EAGAIN, then there's nothing to read (network lag or user not typing), go back to the main loop (also, we shouldn't really have reached this point, because of the select() loop)
* if read() returns 0, we know it's a genuine EOF

or with blocking sockets:

* if select() says the fd has nothing to read, go back to the main loop (done implicitly in GLib/Qt, in practice)
* if read() is incorrectly called with no data anyway, it blocks until there is data (or possibly returns -1 with errno==EINTR if a signal happened)
* if read() returns 0, we know it's a genuine EOF

I don't know how that translates into Qt, though... the QIODevice abstraction actually seems to be making it harder :-(

(In reply to comment #8)
> (In reply to comment #4)
> > > I hope this is the Qt idiom for basename(3), rather than something that does
> > > I/O? What we want here is basename(3) or the Qt equivalent.
> > 
> >  QFileInfo fi("/tmp/archive.tar.gz");
> >  QString name = fi.fileName();                // name = "archive.tar.gz"
> 
> Right, but the point I was trying to make (which I perhaps didn't express very
> well) is the difference between basename(3) and stat(2). Does QFileInfo look at
> the filesystem, or is it just an abstract API for dealing with filenames?
> 
> What we want is an equivalent of basename(3) or Python's os.path.basename(),
> which don't look at the filesystem at all, but instead have the semantics of
> "if we imagined that this was a file path, what would the filename be?"
> 
> From the docs, it appears that QFileInfo is both :-( but fileName() seems to
> work OK without looking at the filesystem.
> 
> Perhaps we shouldn't do the parsing at all, and should just document that API
> users are meant to pass in a bare filename, mentioning QFileInfo::fileName in
> the docs? After all, to us, the CM and the protocol it's just descriptive text
> really.
I checked the code and QFileInfo::fileName does not do any IO at least on unix. Should I still remove the parsing?

> > > > +    PendingVariant *pv = qobject_cast<PendingVariant *>(op);
> > > > +    mPriv->addr = qdbus_cast<SocketAddressIPv4>(pv->result());
> > > > +    debug().nospace() << "Got address " << mPriv->addr.address <<
> > > > +        ":" << mPriv->addr.port;
> > > 
> > > Please detect failures of the qdbus_cast (CM gave us the wrong type back).
> > Do we really need that? We don't make checks in any other place, and actually
> > the channel will do nothing if this happens, mPriv->addr.address.isNull() will
> > be true. If we are going to handle this what should we do? Invalidate the
> > channel?
> 
> Yeah, I think if the CM gives us stupid input, we should respond by closing and
> invalidating the channel (that's the same thing that telepathy-farsight does on
> fatal streaming errors).
> 
> > > I think we discussed the idea of having IFTC automatically drop n initial bytes
> > > if the initialOffset is smaller than we asked for (in particular, if the sender
> > > starts again at the beginning)?
> > > 
> > > IFTC should certainly fail if the initialOffset is *larger* than we asked for,
> > > since that leaves a gap :-P (this can only happen if the CM or sender is being
> > > stupid)
> > I don't remember we discussing this, we certainly discussed about OFTC skipping
> > the first N bytes until initialOffset, which is actually implemented.
> > Anyway I will work on this.
> 
> To be clear: this is to handle the case where the API user says "please start
> from offset 1234", and the CM replies "starting from offset 1024", e.g. because
> the protocol internally works in 1K blocks. In that case I think we should skip
> past those 210 extra bytes before starting to write to the API user's socket.
>
> > Btw fail in what sense? Close the FTChannel?
> 
> Yeah - nothing else would be safe (we're missing a piece of the file!). I think
> we have a TpQt4-specific error string somewhere already that means "CM is being
> stupid", equivalent to TP_DBUS_ERROR_INCONSISTENT in telepathy-glib? If we say
> "please start from offset 1234" and the CM replies "starting from offset 2048",
> we should invalidate the Channel with that error, and close it.
Yep, already implemented :)
 
> > > > +    // in case of sequential devices, we should read everything from it and
> > > > +    // write to the socket. Let's not do this for non-sequential devices as we
> > > > +    // don't want to read a whole file into memory.
> > > > +    if (isConnected() && mPriv->input->isSequential()) {
> > > > +        QByteArray data;
> > > > +        data = mPriv->input->readAll();
> > > > +        mPriv->socket->write(data); // never fails
> > > > +    }
> > > 
> > > How does this work? I suspect that either this is unnecessary because we're
> > > just going to stream until EOF anyway (in which case it's unnecessary for
> > > sequential files too), or it's necessary (in which case we have a bug in the
> > > non-sequential (pipe) case)?
> > There is no way to know EOF is reached for a sequential device. Imagine the
> > following scenario.
> >  - The input is a QTcpSocket or stdin and the transfer did not finished
> > (network lag or user stopped typing) but "EOF" is reached.
> > 
> > For sequential devices the only way to know it finished is either connecting to
> > aboutToClose (the user closed the input device) or using the size (which we
> > can't). Reading from a sequential device will always return 0 if there is
> > nothing to read. For stdin the app could ask the user to type EOF for example
> > and close the input device, and we would know the transfer finished
> > (aboutToClose would be emitted).
> > So you got the idea.
> 
> Hmm. This still raises red flags for me... if we always read as much as is
> available, surely aboutToClose will never be emitted with data still in the
> queue?
In the outgoing case we are not reading everything from input all the time, we read from blocks (16k now) so there may still be data to read.

From Qt QIODevice::aboutToClose docs, "This signal is emitted when the device is about to close. Connect this signal if you have operations that need to be performed before the device closes (e.g., if you have data in a separate buffer that needs to be written to the device)."

I just do this for sequential devices, as we probably don't want to readAll for a file for example, so this was the only way I found to check "EOF" was reached when using sequential devices.

> +        QTcpServer tcpServer;
> +        tcpServer.listen(QHostAddress("127.0.0.1"));
> +        quint16 port = tcpServer.serverPort();
> +        tcpServer.close();
> +
> +        SocketAddressIPv4 addr;
> +        addr.address = "127.0.0.1";
> +        addr.port = port;

This doesn't look right. Does anything guarantee that the kernel will give you the same port when you bind() again? I don't think it does!

You have to do something that results in a call to bind() and getsockname() only, and leaves the socket hanging around for a later outgoing connect() - I have no idea what in Qt does that. It's not a common high-level thing to do, so some high-level APIs (wrongly!) abstract away the difference between bind() and listen()...

Given that Gabble doesn't support this, I think we should revert this change, and file a bug about it instead (and the same for Gabble).

We may be able to support this access-control mode later by extracting the fd from the QTcpSocket, and calling bind() and getsockname() directly?

The rest of your changes up to 715aeab look good.

(In reply to comment #9)
> I checked the code and QFileInfo::fileName does not do any IO at least on unix.
> Should I still remove the parsing?

Keep it, I suppose. It's an easy way to comply with the spec's recommendation.

> > Yeah - nothing else would be safe (we're missing a piece of the file!). I think
> > we have a TpQt4-specific error string somewhere already that means "CM is being
> > stupid", equivalent to TP_DBUS_ERROR_INCONSISTENT in telepathy-glib? If we say
> > "please start from offset 1234" and the CM replies "starting from offset 2048",
> > we should invalidate the Channel with that error, and close it.
> Yep, already implemented :)

It looks as though you just cancel() it?

> In the outgoing case we are not reading everything from input all the time, we
> read from blocks (16k now) so there may still be data to read.
> 
> From Qt QIODevice::aboutToClose docs, "This signal is emitted when the device
> is about to close. Connect this signal if you have operations that need to be
> performed before the device closes (e.g., if you have data in a separate buffer
> that needs to be written to the device)."
> 
> I just do this for sequential devices, as we probably don't want to readAll for
> a file for example, so this was the only way I found to check "EOF" was reached
> when using sequential devices.

Perhaps the right handling is to ignore the sequential/not distinction, on the basis that we have to read the rest of the data for correctness, and we shouldn't get aboutToClose from a file (at least until we've read everything).

(In reply to comment #10)
> > +        QTcpServer tcpServer;
> > +        tcpServer.listen(QHostAddress("127.0.0.1"));
> > +        quint16 port = tcpServer.serverPort();
> > +        tcpServer.close();
> > +
> > +        SocketAddressIPv4 addr;
> > +        addr.address = "127.0.0.1";
> > +        addr.port = port;
> 
> This doesn't look right. Does anything guarantee that the kernel will give you
> the same port when you bind() again? I don't think it does!
> 
> You have to do something that results in a call to bind() and getsockname()
> only, and leaves the socket hanging around for a later outgoing connect() - I
> have no idea what in Qt does that. It's not a common high-level thing to do, so
> some high-level APIs (wrongly!) abstract away the difference between bind() and
> listen()...
> 
> Given that Gabble doesn't support this, I think we should revert this change,
> and file a bug about it instead (and the same for Gabble).
> 
> We may be able to support this access-control mode later by extracting the fd
> from the QTcpSocket, and calling bind() and getsockname() directly?
We will
 
> The rest of your changes up to 715aeab look good.
> 
Reverted changes for now, will file a bug agains gabble and one against tp-qt4.

(In reply to comment #11)
> (In reply to comment #9)
> > I checked the code and QFileInfo::fileName does not do any IO at least on unix.
> > Should I still remove the parsing?
> 
> Keep it, I suppose. It's an easy way to comply with the spec's recommendation.
Ok

> > > Yeah - nothing else would be safe (we're missing a piece of the file!). I think
> > > we have a TpQt4-specific error string somewhere already that means "CM is being
> > > stupid", equivalent to TP_DBUS_ERROR_INCONSISTENT in telepathy-glib? If we say
> > > "please start from offset 1234" and the CM replies "starting from offset 2048",
> > > we should invalidate the Channel with that error, and close it.
> > Yep, already implemented :)
> 
> It looks as though you just cancel() it?
Added invalidated with TELEPATHY_QT4_ERROR_INCONSISTENT (new constant)

> > In the outgoing case we are not reading everything from input all the time, we
> > read from blocks (16k now) so there may still be data to read.
> > 
> > From Qt QIODevice::aboutToClose docs, "This signal is emitted when the device
> > is about to close. Connect this signal if you have operations that need to be
> > performed before the device closes (e.g., if you have data in a separate buffer
> > that needs to be written to the device)."
> > 
> > I just do this for sequential devices, as we probably don't want to readAll for
> > a file for example, so this was the only way I found to check "EOF" was reached
> > when using sequential devices.
> 
> Perhaps the right handling is to ignore the sequential/not distinction, on the
> basis that we have to read the rest of the data for correctness, and we
> shouldn't get aboutToClose from a file (at least until we've read everything).
I would stick with this code, or we would not work with sequential devices in most cases.

Correct:

Use case (with aboutToClose):
  - Input is a socket with 32k size
  - User writes 32k to socket at once
  - Code reads 16k and schedule another read 
  - Input is closed
  - (aboutToClose) Read the other 16k and send to output

Wrong:

Use case (without aboutToClose):
  - Input is a socket with 32k size
  - User writes 32k to socket at once
  - Code reads 16k and schedule another read 
  - Code reads the other 16k and finishes reading, but the transfer is never finished as we don't know EOF is reached

Use case (with aboutToClose just finishing transfer):
  - Input is a socket with 32k size
  - User writes 32k to socket at once
  - Code reads 16k and schedule another read 
  - Input is closed
  - (aboutToClose) finish transfer which is not complete

Filled bugs 24109 and 24110 for gabble/tp-qt4 Port access control support

(In reply to comment #13)
> > It looks as though you just cancel() it?
> Added invalidated with TELEPATHY_QT4_ERROR_INCONSISTENT (new constant)

Oh? I thought we had that already?

> > Perhaps the right handling is to ignore the sequential/not distinction, on the
> > basis that we have to read the rest of the data for correctness, and we
> > shouldn't get aboutToClose from a file (at least until we've read everything).
> I would stick with this code, or we would not work with sequential devices in
> most cases.
> 
> Correct:
> 
> Use case (with aboutToClose):
>   - Input is a socket with 32k size
>   - User writes 32k to socket at once
>   - Code reads 16k and schedule another read 
>   - Input is closed
>   - (aboutToClose) Read the other 16k and send to output

We essentially have two cases, seekable files and non-seekable pipes (I'm not clear on how the Qt terminology works).

Perhaps I was unclear. I think we should do this (the "non-seekable pipe" code path) for *all* objects, including files - if this behaviour is necessary for correctness, then it's necessary for correctness, and we should always do it.

I conjecture that in the seekable file case, aboutToClose will never be emitted with "much" data remaining - so the practical behaviour of the code will be the same as we currently have.

(In reply to comment #15)
> (In reply to comment #13)
> > > It looks as though you just cancel() it?
> > Added invalidated with TELEPATHY_QT4_ERROR_INCONSISTENT (new constant)
> 
> Oh? I thought we had that already?
> 
> > > Perhaps the right handling is to ignore the sequential/not distinction, on the
> > > basis that we have to read the rest of the data for correctness, and we
> > > shouldn't get aboutToClose from a file (at least until we've read everything).
> > I would stick with this code, or we would not work with sequential devices in
> > most cases.
> > 
> > Correct:
> > 
> > Use case (with aboutToClose):
> >   - Input is a socket with 32k size
> >   - User writes 32k to socket at once
> >   - Code reads 16k and schedule another read 
> >   - Input is closed
> >   - (aboutToClose) Read the other 16k and send to output
> 
> We essentially have two cases, seekable files and non-seekable pipes (I'm not
> clear on how the Qt terminology works).
seekable (non sequential), non seekable (sequential)

> Perhaps I was unclear. I think we should do this (the "non-seekable pipe" code
> path) for *all* objects, including files - if this behaviour is necessary for
> correctness, then it's necessary for correctness, and we should always do it.
> 
> I conjecture that in the seekable file case, aboutToClose will never be emitted
> with "much" data remaining - so the practical behaviour of the code will be the
> same as we currently have.
The big difference here is that the user MUST close the input in case of sequential (non seekable) devices in order to finish the transfer, for non sequential devices (seekable) we can check for EOF. Removed the check for sequential on aboutToClose, now we do the same for all devices.

I think this looks OK to merge now.

Fixed upstream, will be in next release 0.1.11