[Bug 35084] tp-qt4: Design and implement API for easily being a StreamTube Handler

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=35084

--- Comment #7 from Olli Salli <ollisal at gmail.com> 2011-09-13 05:07:25 PDT ---
(In reply to comment #5)
> Initial review for current branch state. More to come later.
> 
> +    friend class StreamTubeServer; // interim until STS is implemented using
> tube wrapping
>      TELEPATHY_QT4_NO_EXPORT SharedPtr<RefCounted> _object() const; // TODO:
> turn this into _object()
> Is this still needed?
> 

No :) Just forgot to remove it earlier...

> ++++++ simple-stream-tube-handler
> 
> +        foreach (StreamTubeChannelPtr tube, mTubes.keys()) {
> const StreamTubeChannelPtr &, not a ConstPtr

Huh, ConstPtr? But changed to a ref now anyway.

> +        foreach (QString service, p2pServices)
> const QString &
> 
> +        foreach (QString service, roomServices)
> same here

Refs now too, and so are all other non-primitive foreach loop variables where
the macro implementation allows it (i.e. not QPairs)

> 
> +        Q_ASSERT(!p2pServices.isEmpty() || !roomServices.isEmpty());
> I don't think we should ASSERT here. Just fail ::create and warn if they are
> both empty.
> This is internal API, but it's called directly from STubeClient/Server
> constructors with user provided params.
> 

My bad, I thought I made the public STS/STC create methods do the check and
return null. However, I don't want to return null from the private class
create(), because that'd just complicate the public classes' ctors and
duplicate the check. And actually now that I think of it, there's a valid
usecase for a StreamTubeServer which has an empty filter, which is if you only
use it as the PreferredHandler. Thus, I moved the check to STC, which now
returns a null StreamTubeClientPtr.

> 
> +            const QString channelType =
> +                chan->immutableProperties()[QLatin1String(
> +                        TELEPATHY_INTERFACE_CHANNEL
> ".ChannelType")].toString();
> This reminds me we should parse immutable properties on construction and
> properly return them on accessors when available. So you could use
> chan->channelType() directly here, even if the channel is not ready.

Yeah but we don't :( This would be a minor performance win for Channel initial
state download, but that's a job for another branch. Even more importantly,
StreamTubeChannel should initialize itself from the immutable props, so it
could avoid the Core GetAll completely - which it currently does
unconditionally. Added a TODO anyway to use channelType() here.

> 
> +void SimpleStreamTubeHandler::onReadyOpFinished(Tp::PendingOperation *op)
> +{
> +    Q_ASSERT(!mInvocations.isEmpty());
> +    Q_ASSERT(!op || op->isFinished());
> Well I guess this will break when called from here:
> +        invocation->message = QLatin1String("Got no suitable channels");
> +        onReadyOpFinished(0);

It doesn't. This codepath is thoroughly tested. As a whole SSTH is 97.0% line
coverage at the moment, and is missing just testing a tube getting invalidated
between it having been prepared and other parts of the PendingComposite for it
finishing.

I don't know you think it would break, but I guess because of checking
op->isFinished. That is avoided due to the standard C and C++ rule of
short-circuit evaluation for boolean logic - because !op, the latter argument
to || is not evaluated at all. We make use of this excessively in a whole lot
of places...

> Also here you should properly check that context->setFinished is being called
> when
> onReadyOpFinished(0) is called.

We can't check that a context finishes because of the QDBus bug where local
loop async replies are ignored. But well, it is, because we set an invocation
error.

> You could simplify this by just calling ctx->setFinished/WithError directly
> when no tube channel was provided in handleChannels()

Then the invocations don't finish in order. Currently, the queued finish is
used for everything, which maintains perfect event ordering event for these
corner cases.

> 
> +        foreach (StreamTubeChannelPtr tube, invocation->tubes) {
> +            if (!tube->isValid()) {
> const StremTubeChannelPtr &, not a ConstPtr
> Please check other loops also for similar "issues".

Umm, again, I don't know what you mean by ConstPtr, but fixed along with the
other foreaches.

> 
> Also it seems InvocationData leaks here.

Care to elaborate? All InvocationData pointers are put directly to a RAII
SharedPtr when creating them, so they can't leak. Also, valgrind shows no
memory leaks from any tp-qt4 code at least in the current StreamTubeHandlers
tests, which is, again, already at 97.0% coverage for SSTH.

> 
> Even SimpleStubeHandler being internal I would like to have header guards
> there.

Sure, just seem to have forgotten them.

> 
> 
> ++++++ stream-tube-client
> 
> +    foreach (TubeWrapper *wrapper, mPriv->tubes.values()) {
> +        wrapper->deleteLater();
> +    }
> This could be avoided by just passing this as parent when creating the wrappers

Ah indeed, good idea. Done.

> 
> +        tube->requestClose();
> +        return;
> +    }
> +
> +    TubeWrapper *wrapper = 0;
> +    
> The last line contains extra EOL spaces

Fixed, thanks for spotting

> 
> Please add accessors for "does it require credentials and is using unix
> sockets" and 

There is acceptsAsUnix, and the tubeAcceptedAsUnix signal carries the info on
whether the credentials are needed already along with the credential byte which
was generated if they are. This is the only credential-requiring information
which makes sense; for connecting to a tube, it matters what the STC's settings
were when a tube was accepted, not what the settings for future tubes are at
the time an application gets the notification for a tube being accepted.

> rename const TcpSourceAddressGenerator *generator() const; to something more
> sensible like
> tcpGenerator();

Renamed

> 
> Also I think we should default to accept as unix without credentials. 

We can't "default" to anything, because as soon as we register the handler
object, we can be invoked. If we set ourselves to accept as Unix upon
construction and registered ourselves, if the user actually wanted TCP sockets
or Credentials AC, they'd need to know to export however they want at the exact
same mainloop iteration as creating the STC, otherwise the first (few)
channel(s) might be incorrectly accepted. Alternatively, if we just "defaulted
to accepting as unix" but didn't register, we'd need an explicit register call,
at which point defaulting serves no purpose - no few API calls needed, but it's
just needlessly obscure which the settings are in the "default" case.

> +    Q_ASSERT(isRegistered());
> The current code will crash if onInvokedForTube is called and setToAcceptAs*
> hasn't being called yet or registerClient failed for some reason.

It is not called in that case, because the handler is not registered on the bus
(so it can't receive channels and consequently emit onInvokedForTube). If that
nevertheless happens, it's a bug in SSTH (it e.g. prematurely registers itself
or spuriously emits false events) - which is exactly what I tried to catch here
with this assert. Added comments explaining this shortly to these asserts in
STS and STC.

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