[Bug 23819] Add high-level API for FileTransfer

[bugzilla-daemon at freedesktop.org]

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





--- Comment #4 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-09-22 18:52:50 PST ---
(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 


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