[Bug 23819] Add high-level API for FileTransfer

[bugzilla-daemon at freedesktop.org]

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





--- Comment #9 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-09-23 05:47:50 PST ---
(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.


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