[Bug 23819] Add high-level API for FileTransfer

[bugzilla-daemon at freedesktop.org]

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





--- Comment #8 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-23 04:22:06 PST ---
(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 :-(


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