[Bug 23819] Add high-level API for FileTransfer

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=23819





--- Comment #3 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-22 10:59:17 PST ---
(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.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.