Add support for the Tube interface.
Dario has branches for both StreamTubes and DBusTubes, which are sadly bitrotting now. Let's make merging them a priority after the API/ABI break dust settles.
Good news. When you will be ready for reviewing please ping me, so that I can rebase the branches onto master one last time given the hard changes, and make them work again.
StreamTube support is merged, so this bug pretty much becomes restricted to the D-Bus tubes part. I think a reasonable way forward (as the QDBus P2P support is STILL not merged I think ?!?) would be to perfect the API WITHOUT offering API to support passing QDBusServer / retrieving QDBusConnection from it. Similarly to the stream tubes case, we need such non-Qt-infra API as well anyway for connecting applications/libraries not exposing QDBus in their API. Granted, for an application otherwise written using tp-qt4, it's far less likely for it to have a part which needs to be connected using, say, dbus-glib, but it's still possible. We can add the QDBus accessors later when the QDBus support actually works.
Cool. Will start working on this today and will keep you updated.
You probably expected a beer for St. Patrick's, unfortunately I just have a branch for review. The name is "dbus-tubes". Some small nitpicking: the high-level wrappers are just mere containers for handling accept and offer methods, they might become more useful whenever we'll add methods for returning Qt objects such as QDBusServer. I introduced 3 pending operations which I made public, but can be made private if necessary.
(In reply to comment #5) > You probably expected a beer for St. Patrick's, unfortunately I just have a > branch for review. The name is "dbus-tubes". > > Some small nitpicking: the high-level wrappers are just mere containers for > handling accept and offer methods, they might become more useful whenever we'll > add methods for returning Qt objects such as QDBusServer. > I introduced 3 pending operations which I made public, but can be made private > if necessary. P.S.: My fd.o account is now back and alive, so I can merge stuff myself from now on
Dario has had a branch at the URL for some time now. I originally dismissed it because it wasn't beer. Sorry about that. I've had enough beer since, so it's review time. > handling accept and offer methods, they might become more useful whenever we'll > add methods for returning Qt objects such as QDBusServer. Actually I think we should only ever return QDBusConnections. The CM implements the p2p dbus transport server side for both incoming and outgoing dbus tubes. (That's why both accept and offer return an address to the CM server, unlike stream tubes where you offer a server of your own). The QDBusConnection accessor doing connectToBus() can then co-exist with the QString address accessor in the DBusTubeChannel base class. As the paradigm is the same for both incoming and outgoing tubes, I'd use just one Tp::PendingDBusTube class which waits for accept/offer to finish and the tube to go Open (or fail doing that), to avoid duplication in both the API and the implementation. Or perhaps you only need to return PendingOperation, with the address/connection accessible through the Tp::DBusTubeChannel instance once that finishes? Please add a DBusTube channel class to RequestableChannelClassSpec, and using it, capability accessors similar to the stream tube ones to ContactCapabilities and ConnectionCapabilities (with service name instead of service). Or is this somehow not feasible? > static const Feature FeatureBusNamesMonitoring; FeatureBusNameMonitoring would be more proper English. Otherwise, the public APIs look good to me. > path: root/TelepathyQt4/dbus-tube-channel-internal.h We only use separate internal headers if we need to run moc on the private class(es), which is not the case here. > busNames.insert(parent->connection()->contactManager()-> > lookupContactByHandle(i.key()), i.value()); This won't work in the general case, as we discussed back when the stream tube channel API was being implemented - you have to asynchronously build the contacts instead. I noticed that the lookupContactByHandle function is nowadays public: this is totally incorrect (Andre agrees). I've filed bug 37748 as a result. if (parent->targetHandleType() == static_cast<uint>(Tp::HandleTypeRoom)) { parent->connect(dbusTubeInterface, SIGNAL(DBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList)), parent, SLOT(onDBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList))); } else { As you weren't following the signal before, you might have lost changes to it since the initial property download. Remember that additional features can be activated later, not only in one bunch when making an initial becomeReady call. Hence you must refresh the current state of the property after connecting by requestPropertyDBusNames'ing it, and only finish the feature introspection once that is done. Additionally, I don't like the fact that whatever DBusNames were there at the time of the original introspection of FeatureDBusTube will be stuck (e.g. contacts reffed) in the private busNames variable forever even if they leave if FeatureBusNameMonitoring is not activated. It should only be populated when enabling that feature in the first place. * Returns the service name which will be used over the tube. This should be a * well-known and valid DBus service name, in the form "org.domain.service". Tubes * providing invalid service names might cause non-predictable behavior. * This comment in DBusTubeChannel::serviceName() rather makes more sense when specifying the service name when requesting a channel. A properly requested channel will always have such a service name set. * If the tube has been opened, this function returns the private bus address you should be listening * to for using this tube. You don't listen to the tube, you connect to it. * This function returns all the known active connections since FeatureConnectionMonitoring has * been enabled. For this method to return all known connections, you need to make * FeatureConnectionMonitoring ready before accepting or offering the tube. Accidentally refers to a StreamTubeChannel property, in addition to the quite awkward semantics resulting from not reintrospecting the bus names property properly. I'd also like a mention here that this will be empty for p2p channels no matter what one does, because it's trivial to deduce the bus names for ourselves and the other party. Relatedly, I'd make warning() << "FeatureBusNamesMonitoring does not make sense in a P2P context"; debug only, as one can't easily know whether a tube is a group tube or p2p one before introspecting Channel::FeatureCore, which will usually only be introspected at the same time as this feature. mPriv->extractProperties(props); debug() << "Got reply to Properties::GetAll(DBusTubeChannel)"; Please reverse these lines; that way there's a hint that we're crashing in the response handling if the extraction is buggy. General rule: report the D-Bus event that causes logic to be run before running the logic. > void DBusTubeChannel::onDBusNamesChanged(const Tp::DBusTubeParticipants &added, Queue, queue. Also the meaning of the "real" sets seems to be lost here: you're intended to weed out redundant events (such as adding somebody who already is there, or removing somebody who wasn't there in the first place) using them. Check the other code using "real" sets in more detail for reference. * \fn void DBusTubeChannel::busNamesChanged(const QHash< ContactPtr, QString > &added, const QList< ContactPtr > &removed) * * Emitted when the participants of this tube change Only for multi-user tubes, and when the feature has been enabled. * \c IncomingDBusTubeChannel is an high level wrapper for managing Telepathy interface * #TELEPATHY_INTERFACE_CHANNEL_TYPE_DBUS_TUBE. * In particular, this class is meant to be used as a comfortable way for accepting incoming requests. Well not quite. This description gives the hint that one'd use this in the Approver for accepting/rejecting incoming tubes. One doesn't use it for that: the Handler uses it for getting a bus connection to start communicating with when somebody offers you a tube (and it has been accepted by an Approver, if there is one). * Tp::IncomingDBusTubeChannelPtr::dynamicCast(channel); Promote qObjectCast instead of dynamicCast, please. I wonder if we should have the handleChannels example at all, actually. First of all, it shows nothing which wouldn't be applicable to any other kind of channel (it's after all just casting to the subclass based on the channel class), second: we should finish bug 35085 and promote using it instead of implementing custom handlers. This reminds me, whoever continues bug 35084 should also remove any similar docs from StreamTubeChannel... The feature preparation things are likewise in conflict with 1) the practice of using factories currently considered ideal and 2) the fact that the bug 35085 API should do this for you anyway. 2) applies to the acceptTube doc as well. * To learn more on how to use introspectable and features, please see \ref account_ready_sec. account? * QString address = op->address(); * // Do some stuff here Well, some stuff would be to connect some kind of dbus transport (such as QDBusConnection, or DBusGConnection) to that address. This part will remain even after bug 35085 API has been added: it'll just signal you addresses to connect to, or after Qt 4.8 is released, QDBusConnections if you tell it you want those. * * \note When using QHostAddress::Any, the allowedPort parameter is ignored. * acceptTube doesn't have such a parameter anyway, unlike the StreamTubeChannel accept as tcp socket method. I'd also like to hint that you can only use credentials if you can tell your dbus transport (unlike, say, QDBus's) to actually pass the SCM_CRED(ENTIAL)S message. The actual accept method impl looks fine. The example considerations from ISTC apply to OutgoingStreamTubeChannel as well. It's cool to direct people to use Account to request the channel, though! EXCEPT: * Be sure to track the pending request to retrieve your outgoing DBus tube upon success. The Handler gets the tube, not any arbitrary "you". In particular, people should not try to implement tube apps by using the PendingChannel::channel() accessor, which is provided strictly for observation purposes (and documented thus), and then acting like they were the tube Handler. Once again, once bug 35085 is done, in most uses the request should just eventually result in the DBusTubeServer or alike class giving you an address/connection to export your object on, or perhaps exporting it for you. * This method creates a brand new private DBus connection, and offers it through the tube. * Well it moreso sets up a private DBus connection to the channel target(s). * \param parameters A dictionary of arbitrary Parameters to send with the tube offer. * Please read the specification for more details. There's not that much more to read in the spec. It'd suffice to say that this is what ends up in parameters() of the corresponding IncomingStreamTubeChannel in the other end. In PendingDBusTubeAccept::onTubeStateChanged(): // The tube is ready: let's inject the address into the tube itself This comment looks a bit stale, as the address has been injected earlier on. Otherwise the pending operations look OK, except for the aforementioned needless duplication between the two, and it being questionable to have a public class for them at all because you can anyway get the same information from the channel object once the PendingOperation has finished.
(In reply to comment #7) > Dario has had a branch at the URL for some time now. I originally dismissed it > because it wasn't beer. Sorry about that. He's updated it. I'll re-review now. Hopefully it's not WIP at this point or anything - I guess I'll find out in a bit. Commenting on my own post so it acts sort of like a checklist for having taken care of everything. > As the paradigm is the same for both incoming and outgoing tubes, I'd use just > one Tp::PendingDBusTube class which waits for accept/offer to finish and the > tube to go Open (or fail doing that), to avoid duplication in both the API and > the implementation. Or perhaps you only need to return PendingOperation, with > the address/connection accessible through the Tp::DBusTubeChannel instance once > that finishes? This has now been done, and sure enough the logic is exactly the same for both. However, there still is a separate onOfferFinished and onAcceptFinished, and separate constructors - why are these needed? The only difference I can see in them is the different debug message (which could be moved to the "not open yet" case and say something like "Waiting for the tube to be opened", as the other case debugs anyway already). So I'd rather remove this duplication too. However, I see another issue now: PendingDBusTube::onTubeStateChanged is expecting the tube state changes to either local pending or open. However, for outgoing tubes it surely won't ever change to local pending; but it WILL change to remote pending until the remote accepts it, which would currently trigger the "error handling": The code seems to be using the other states as some kind of error mechanism; this is incorrect. There is no "failed" state. Rather, the class should connect to the invalidated signal of the channel: if the offer is rejected or some other error happens, the channel will be invalidated. I note PendingStreamTubeConnection also expects to receive the errors through some bogus tube state change: that won't happen. Could fix that as well, please, by additionally connecting to the channel's invalidated signal when it starts waiting for a state change? > > Please add a DBusTube channel class to RequestableChannelClassSpec, and using Done-ish, however: +RequestableChannelClassSpec RequestableChannelClassSpec::dbusTube(const QString &serviceName) +{ + static RequestableChannelClassSpec spec; + + if (!spec.isValid()) { + RequestableChannelClass rcc; + rcc.fixedProperties.insert(TP_QT4_IFACE_CHANNEL + QLatin1String(".ChannelType"), + TP_QT4_IFACE_CHANNEL_TYPE_STREAM_TUBE); Compare the first and last lines please :P > it, capability accessors similar to the stream tube ones to ContactCapabilities > and ConnectionCapabilities (with service name instead of service). Or is this > somehow not feasible? Not done! Oh well, I guess this was still WIP anyway then. > > > static const Feature FeatureBusNamesMonitoring; > > FeatureBusNameMonitoring would be more proper English. Fixed now, good! > > Otherwise, the public APIs look good to me. > > > path: root/TelepathyQt4/dbus-tube-channel-internal.h > > We only use separate internal headers if we need to run moc on the private > class(es), which is not the case here. Good, you've fixed this. As a bonus this made the address setting better decoupled and thus cleaner. > > > busNames.insert(parent->connection()->contactManager()-> > > lookupContactByHandle(i.key()), i.value()); > > This won't work in the general case, as we discussed back when the stream tube > channel API was being implemented - you have to asynchronously build the > contacts instead. I noticed that the lookupContactByHandle function is nowadays > public: this is totally incorrect (Andre agrees). I've filed bug 37748 as a > result. This is now mostly done, however there's a small issue: + // Add it to our connections hash + foreach (const Tp::ContactPtr &contact, contacts) { + mPriv->busNames.insertMulti(contact, busName); + added.insert(contact, busName); + } Aside from the copy-pasted comment (we have bus names here, not connections), you shouldn't be using insertMulti: each contact can only ever have one bus name, as the DBusNames property isn't a multi-map (map with lists as values) either. Also, you shouldn't emit a signal if a removed contact wasn't in the current bus names set: you should rather print a warning in that case, because the service is misbehaving. The rest is still not done I see. OK, so this was quite WIP.
Updated - for real :P I think I addressed everything, including your post-comments, but I surely missed something on my way. Btw, close enough I think. Besides that, my fdo account is now working, so when ready, I'll be able to merge that myself.
(In reply to comment #8) > This has now been done, and sure enough the logic is exactly the same for both. > However, there still is a separate onOfferFinished and onAcceptFinished, and > separate constructors - why are these needed? The only difference I can see in The slot is now shared, which is good, but you still have the two identical constructors. You've now fixed the invalidation vs bogus state change issue in PendingDBusTube, but I believe the similar bug in the PendingStreamTubeConnection class remains. Could you fix that too, please? This branch or a separate one doesn't really matter. + * This signal will be emitted only if the tube is a group tube (not p2p), and if the + * FeatureBusNameMonitoring feature has been enabled. You usually want to wait until the + * aforementioned feature is ready before connecting to this signal. What's the reason for which one would want to wait? I can't think of any. + * Return whether creating a DBusTube channel, using the given \a serviceName, by providing a + * contact identifier is supported. As docs for ContactCapabilities::dbusTubes() this is not very appropriate: it's not about "providing a contact identifier": these ContactCapabilities are specific to *this particular contact*. So something like "Return whether creating a DBusTube channel with the given service targeting this contact is expected to succeed\n". I see the corresponding StreamTube method description is also similarly misleading, could you fix that as well? + } else { + warning() << "Trying to remove a bus name which has not been retrieved previously!"; + } You could print the bus name in question. Now as for the stuff which is still missing/broken: * Be sure to track the pending request to retrieve your outgoing DBus tube upon success. These kinds of very, very misleading (given how the channel requesting and dispatching big picture really works) comments are still there in both OutgoingDBusTubeChannel and OutgoingStreamTubeChannel. Also, the long descriptions are still promoting the use of explicit becomeReady calls over using the factories. And linking to the account readiness section - why not the general async model section, for example? If you rebase on top of current master, you'll find the other documentation in this area improved a lot. * \note When using QHostAddress::Any, the allowedPort parameter is ignored. * This is also still there in the acceptTube docs, although there is no such parameter. Actually, please go through my initial comments on the documentation again in general to see if you missed anything else like these. DBusTubeChannel::FeatureBusNameMonitoring still has absolutely horrible semantics, namely: - if you don't enable it, you'll get a random initial set of names, which doesn't stay up to date, forever stuck in busNames() - if you enable it later than you enabled FeatureCore, you have lost all changes in between - and actually, as even with a single becomeReady call / using factories with both set, FeatureBusNameMonitoring is only introspected after Core, there is a race condition: you'll in any case lose whichever changes happen between introspecting Core, and getting connected to the change notification signal when introspecting Monitoring! You *MUST* only request/extract the property *after* connecting to the change notification signal, in the introspection function for FeatureBusNameMonitoring. This way all the above issues will be fixed. You can use the "new" requestPropertyDBusNames() method for nicely getting the single property once needed. The above change to not make FeatureCore consider the DBusNames property at all also has a favorable consequence: the remaining two properties are actually immutable (although somebody has forgot to write that in the spec. Anyway, they don't have change notification signals or any other way to change.). This means that if the base class Channel's immutableProperties() has those properties, you don't need to call GetAll at all for Core, just extract them. In case you feel like improving the code further, while doing the introspection changes to properly introspect DBusNames and consider immutableProperties() for the remaining Core properties, you could start using DBusTubeChannelInterface::requestAllProperties() instead of an explicit PropertiesInterface::GetAll call.
Due to the insurmountable amount of bugs encountered in the similar stream tubes code, I'm adding the requirement that this needs unit tests before it can be merged. Telepathy-Glib test suite's stream-tube-chan.[ch] demonstrates that one can write a Tube test service just fine, so that is no excuse - you can probably adapt that test service for testing DBus tubes too. Note that you don't have to emulate a dbus daemon in the test service, as the TpQt4 classes tested aren't going to connect to a D-Bus bus anyway - but you should by other means verify that their operations signal the address which the CM is listening on (as a bogus no-op service) properly.
NOTE: The branch has been rebased on top of master. Everything has been fixed, except: (In reply to comment #10) > + } else { > + warning() << "Trying to remove a bus name which has not been > retrieved previously!"; > + } > > You could print the bus name in question. Actually no, as I just get a list of contacts. If the contact is not associated to any bus name, I cannot retrieve it - I can print the contact id though. For the rest, I'll provide unit tests with the next update
(In reply to comment #12) > NOTE: The branch has been rebased on top of master. > > Everything has been fixed... + * DBusTubeChannel features can be enabled by constructing a ChannelFactory and enabling the desired features, + * and passing it to ChannelRequest or ClientRegistrar when creating them as appropriate. However, No you don't ever create ChannelRequest objects yourself. The channel factory has been passed to the AccountManager (or in rare cases a single Account created directly) in this use case. It suffices to say AccountManager like the rest of the proxies do. // It makes sense only if this is a room, if that's not the case just spit a warning if (parent->targetHandleType() == static_cast<uint>(Tp::HandleTypeRoom)) { parent->connect(dbusTubeInterface, SIGNAL(DBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList)), parent, SLOT(onDBusNamesChanged(Tp::DBusTubeParticipants,Tp::UIntList))); } else { debug() << "FeatureBusNameMonitoring does not make sense in a P2P context"; } // Request the current DBusNames property connect(dbusTubeInterface->requestPropertyDBusNames(), SIGNAL(finished(Tp::PendingOperation*)), parent, SLOT(onRequestPropertyDBusNamesFinished(Tp::PendingOperation*))); You don't need to make the D-Bus call to fetch the DBusNames property either if the handle type isn't Room. Just finish the feature directly in the else branch. Another, more serious flaw is that you currently finish the feature before the contacts/names actually appear in busNames(), as they go around the queued contact builder thingy. Channel only finishes its introspection when all the initial member contacts have been built and appeared in the member sets, for example. You should do the same; I think that would be most easily accomplished by ignoring bus name change signals until you get the initial bus names property value (which you should do anyway), and then finishing the feature in the contact build slot once pendingNewBusNamesToAdd becomes empty. Watch out for the case where there are no initial bus names though, and be sure to test both cases, and bus name changes during introspection, in your unit tests. + if (parent->immutableProperties().contains(QLatin1String("Service")) && + parent->immutableProperties().contains(QLatin1String("SupportedAccessControls"))) { This won't ever be true, because immutableProperties() are fully qualified property names (the property name is prefixed with the interface name and a '.'). As a fix, check for fully qualified names here, make extractProperties take a map with fully qualified property names, and prefix the properties you get in onRequestAllPropertiesFinished with the interface name to make them the same format as the immutable props. > , except: > (In reply to comment #10) > > + } else { > > + warning() << "Trying to remove a bus name which has not been > > retrieved previously!"; > > + } > > > > You could print the bus name in question. > > Actually no, as I just get a list of contacts. If the contact is not associated > to any bus name, I cannot retrieve it - I can print the contact id though. Ah, true, please print the contact id indeed. > > For the rest, I'll provide unit tests with the next update Ok, looking forward to that.
Whoops, forgot to write here. All of the issues in the branch have been fixed and pushed, except for docs, for which I'm waiting for Andre's mail as you previously suggested. Unit tests are kind of a problem. Tp-glib has no class for implementing dbus tubes tests, and I wonder if I should take care of that and try to create it, or act differently.
(In reply to comment #14) > Whoops, forgot to write here. All of the issues in the branch have been fixed Yes, the API and code look basically okay now. > and pushed, except for docs, for which I'm waiting for Andre's mail as you > previously suggested. > Well, that was mostly for stream tubes, as that's what Andre has been working on lately. But yeah, some of his suggestions will surely apply to the DBus tube class documentation as well. > Unit tests are kind of a problem. Tp-glib has no class for implementing dbus > tubes tests, and I wonder if I should take care of that and try to create it, > or act differently. Well. There is no ready made test service, no. That's because there is no high-level client-side API for dbus tubes in tp-glib either, so nothing requires such a service for testing. This only means that you have to create the test service yourself, probably by modifying tp-glib's StreamTube channel test service as I mentioned in a previous comment. tp-glib surely does have the needed low-level service classes for implementing a dbus tube channel (one could perhaps look at gabble for reference on how to use that). There's also TpGroupMixin for implementing the Group interface for testing the bus name monitoring stuff in addition to implementing the basic Channel stuff by deriving from TpBaseChannel. In any case, automated regression tests are a must for this code. The fact that there is no ready-made test service already written by someone else is no excuse against that; after all we wouldn't have any unit tests at all if somebody didn't write the test service at first.
(In reply to comment #15) > Well. There is no ready made test service, no. That's because there is no > high-level client-side API for dbus tubes in tp-glib either, so nothing > requires such a service for testing. > > This only means that you have to create the test service yourself, probably by > modifying tp-glib's StreamTube channel test service as I mentioned in a > previous comment. tp-glib surely does have the needed low-level service classes > for implementing a dbus tube channel (one could perhaps look at gabble for > reference on how to use that). There's also TpGroupMixin for implementing the > Group interface for testing the bus name monitoring stuff in addition to > implementing the basic Channel stuff by deriving from TpBaseChannel. > > In any case, automated regression tests are a must for this code. The fact that > there is no ready-made test service already written by someone else is no > excuse against that; after all we wouldn't have any unit tests at all if > somebody didn't write the test service at first. That's ok, I just wanted to check I was not stepping on other people's toes or taking over work I shouldn't have done myself. I'll start researching/implementing that tomorrow, then we'll eventually consider to port this back to tp-glib if successful.
Guess who's back? The branch is still alive. I have added also a first bare version of the Client/Server interface, and rebased it atop of master. I am writing unit tests as we speak, but an early review would be really appreciated. Documentation is fully WIP. P.S.: PendingDBusTube -> PendingDBusTubeConnection for consistency?
Created attachment 53154 [details] lcov report
I finished autotests for this branch and pushed them to dbus-tubes (in the meanwhile, handler/client/server have been moved to a different branch, dbus-tubes-client-server). The lcov report has been attached, and it shows test coverage is almost 100% (and covers all of the functions we're interested in).
P.S.: I had to extend quite a bit the glib service, mostly by taking "inspiration" from gabble, if glib people are interested.
(In reply to comment #19) > The lcov report has been attached, and it shows test > coverage is almost 100% (and covers all of the functions we're interested in). Well, AFAICS it ranges from 73.3% for OutgoingDBusTubeChannel to 90.6% for PendingDBusTubeConnection. Let's say it's manageable - the rest being corner case early outs. Which would be easy enough to test, though - so please green them as well in ODTC and DTC, to get the total coverage up. I mean the early-outs for "you requested credentials checking but it's not supported", "you used a feature without preparing it" and the like. Something even more important to test would be the rather non-trivial contact build code for the DBusNames. That's currently not tested in DTC. Btw, on the subject of credential checking - did you verify that it makes sense for the API user to decide whether the SCM_CREDS message should be required? The potential forces which might make this nonsense are: 1) dbus unix socket transport might always require the message on platforms where it is supported(!!) - or is it gabble which gets to decide whether it requires it or not? 2) in turn, many (all?) transport libraries in the CONNECTING side might offer no way to force or prevent passing it - so then the API user's decision would become "OK I'll find out what my dbus transport library does in its internals, for the platforms I want to support". It's distinctly better if WE figure this out in behalf of the users, being in a much better position expertise wise to do that. Here's a few things to consider for now. I'll post a full review of the current state of the APIs and implementation tomorrow.
(In reply to comment #21) > (In reply to comment #19) > > The lcov report has been attached, and it shows test > > coverage is almost 100% (and covers all of the functions we're interested in). > > Well, AFAICS it ranges from 73.3% for OutgoingDBusTubeChannel to 90.6% for > PendingDBusTubeConnection. Tests have been updated to be all >91%. I also took the chance of greening out TubeChannel, which is >90% as well now. > Something even more important to test would be the rather non-trivial contact > build code for the DBusNames. That's currently not tested in DTC. This is now tested, and a bug in how the busNamesChanged signal was handled has been fixed. > > Btw, on the subject of credential checking - did you verify that it makes sense > for the API user to decide whether the SCM_CREDS message should be required? > The potential forces which might make this nonsense are: > 1) dbus unix socket transport might always require the message on platforms > where it is supported(!!) - or is it gabble which gets to decide whether it > requires it or not? > 2) in turn, many (all?) transport libraries in the CONNECTING side might offer > no way to force or prevent passing it - so then the API user's decision would > become "OK I'll find out what my dbus transport library does in its internals, > for the platforms I want to support". It's distinctly better if WE figure this > out in behalf of the users, being in a much better position expertise wise to > do that. TBH, I'm not really getting this part. DBus can indeed choose whether or not to require credentials, and the default behavior is yes. Gabble indeed offers the possibility of choosing. Changing the default to true seems a good pick to me - but I'm not sure if removing the choice makes sense. Or I misunderstood your point? > > Here's a few things to consider for now. I'll post a full review of the current > state of the APIs and implementation tomorrow. Thank you :)
(In reply to comment #21) > Btw, on the subject of credential checking - did you verify that it makes sense > for the API user to decide whether the SCM_CREDS message should be required? The D-Bus wire protocol always has one '\0' byte at the very beginning of the stream, which is not part of the authentication handshake (text-based SASL) and also not part of any of the subsequent D-Bus messages (binary). libdbus, GDBus etc. assume that this byte will always be present. If credentials-passing via SCM_CREDS (FreeBSD) or SCM_CREDENTIALS (Linux) is supported, the credentials accompany that '\0' byte. If the platform can't pass credentials (Windows) or can pass/get credentials without sending a byte (Linux and OpenBSD via SO_PEERCRED, some other platforms (Solaris?) via getpeereid or getpeerucred), it must still send the '\0' byte. On OSs where you can choose whether to send credentials out-of-band (LOCAL_CREDS - I don't know which platforms actually have this?), D-Bus expects you to say "yes" to LOCAL_CREDS too. Basically, you always send '\0' accompanied by as much out-of-band information as possible (which might be none, e.g. on Windows). > 1) dbus unix socket transport might always require the message on platforms > where it is supported(!!) - or is it gabble which gets to decide whether it > requires it or not? I don't think all Socket_Access_Control values really fit on a (new-style) D-Bus tube - we wrote the wording for stream tubes, so they'll need re-purposing for D-Bus tubes. DBusTube is under-specified, basically. D-Bus connections always start with a '\0' with semantics similar to the Socket_Access_Control_Credentials byte - that's exactly where I got the idea for S_A_C_C from. I think the values for S_A_C that make sense for D-Bus tubes are: * Localhost: any local user can connect to the CM. I'd re-interpret this as "use dbus_connection_set_unix_user_function() and dbus_connection_set_windows_user_function() to set a function that allows everyone". * Credentials: for D-Bus I'd either re-interpret this as "use the default D-Bus auth handshake as used for the session bus, which only allows the same uid; omit the extra byte", or deprecate it for D-Bus tubes (it's fine to use on stream tubes) and introduce a new S_A_C_DBus_Same_User which is explicitly "use the normal D-Bus mechanisms to determine that it's the same user".
(In reply to comment #23) > * Credentials: for D-Bus I'd either re-interpret this as > "use the default D-Bus auth handshake as used for the session bus, > which only allows the same uid; omit the extra byte", or deprecate it > for D-Bus tubes (it's fine to use on stream tubes) and introduce a > new S_A_C_DBus_Same_User which is explicitly "use the normal D-Bus > mechanisms to determine that it's the same user". FWIW, when we designed old-style Tubes channels, D-Bus tubes didn't have an access-control parameter at all - they implicitly had the semantics of "use the normal D-Bus mechanisms to only allow the same user".
> DBusTube is under-specified, basically. I opened Bug #42809 for this.
Thanks, Simon, for the exhaustive reply on the use of credentials passing in DBus. (In reply to comment #22) > TBH, I'm not really getting this part. DBus can indeed choose whether or not to > require credentials, and the default behavior is yes. Gabble indeed offers the > possibility of choosing. Changing the default to true seems a good pick to me - > but I'm not sure if removing the choice makes sense. Or I misunderstood your > point? > From Simon's excellent summary, we can conclude that we can sensibly offer the parameter. Also, as connecting as a different user is seldom useful, we should indeed change the default to S_A_C_Creds here. However, as the DBus concept is that of restricting connections to those from a particular user - and the use of SCM_CREDS on some platforms is just an implementation detail - we should change the terminology in our public API accordingly. That would mean changing the parameter to "allowOtherUsers" with inverted logic, and the capability checker to just "supportsUserAccessControl" (no ambiguity) or alike. Obviously document the (inverse) relationship between the two. We should also document that the choice of using UID access control or allowing all users must be communicated to the dbus transport used as well. I expect that in most uses, people would not care at all if such access control is in use, and use the dbus transport defaults (restrict to single user if possible but be able to function even if not). Thus we should perhaps change trying to use the default (enable S_A_C_Creds) when there is no support (e.g. Windows?) from an hard error to a warning, to match the dbus behavior. People *really* needing stringent security can either assert (forgoing tubes support these platforms) or otherwise bail out on their own, by checking the capability accessor before trying to accept / offer the tube. This matches the behavior of StreamTubeClient: it doesn't drop tubes on the floor or fail by itself even for ones that don't support Creds (or Port in the case of TCP), but allows the user to reject the tube if other local users eavesdropping are a bigger worry than fulfilling the app's functionality goals.
(In reply to comment #22) > Tests have been updated to be all >91%. I also took the chance of greening out > TubeChannel, which is >90% as well now. > ... now tested, and a bug in how the busNamesChanged signal was handled has > been fixed. Great! This level of line coverage is excellent and obviously sufficient for all of our needs. And nice that expanding it also allowed you to squash a real bug :) You could however check that an appropriate error is returned for particular corner cases. expectFailure() stores it in mLastError. The errors you're currently returning from the early-outs seem to be spot on (except that the NOT_IMPLEMENTED early-out we might want to replace with something more graceful entirely). > > Here's a few things to consider for now. I'll post a full review of the current > > state of the APIs and implementation tomorrow. > > Thank you :) So here it goes. + gchar *service = g_strdup("org.not.seen.yet"); doesn't this leak? such an issue exists in +void TestDBusTubeChan::testExtractBusNameMonitoring() and the other bus name monitoring tests. In general, please verify using the check-valgrind(-DBusTubeChannel) targets that there are no "definitely leaked" leaks coming from the test code, so that we can use check-valgrind to check that the tp-qt4 library proper doesn't leak. An another general observation I'd like to make on the test: it seems rather heavily copy-pasted from the stream tube tests. Thus, it has awkward naming for some things, such as mGotRemoteConnection and mGotConnectionClosed, which makes the logic hard to follow as you have to try and figure out which DBus tube concepts they do happen to map to. Please revisit the naming to make it more appropriate for what's actually being tested here. Please also revisit the suggestions I made earlier for the documentation. And well, in general, read the documentation you've written as a whole by yourself. And convince yourself whether it makes sense in this day and age. I still see things like: * DBusTubeChannel features can be enabled by constructing a ChannelFactory and enabling the desired features, * and passing it to ChannelRequest or ClientRegistrar when creating them as appropriate... something which I already mentioned is quite in conflict with reality. And perhaps anyway rather redundant with the general descriptions of how factories and features interact. Note that this is just one example, the docs could use cleanup otherwise. PendingDBusTubeConnection: bool requiresCredentials() const; Let's change this similarly to how asking for it when accepting/offering changes. uchar credentialByte() const; And remove this, because, as Simon mentioned, the cred byte must always be '\0', and the transport sends it for you. DBusTubeChannel: QHash<Tp::ContactPtr, QString> busNames() const; Mm. Could we change this to be the other way around (contact for bus name)? That's what the OutgoingStreamTubeChannel's two maps effectively are as well. The rationale being that apps tend to want to do things like displaying the face of whoever they're communicating with, instead of displaying bus names (source addresses for stream tubes) of people who might have one in a tube. I wonder why the D-Bus construct is a{us} in the first place. But well, that doesn't need to constrain us in making our API intuitive and efficient. Of course, because of this if you reverse the map, you need to O(n) remove the mappings by value, but they're removed only once, and inspected an arbitrary number of times. IncomingDBusTubeChannel: if (mPriv->initRandom) { qsrand(QTime::currentTime().msec()); mPriv->initRandom = false; } credentialByte = static_cast<uchar>(qrand()); accessControlParam.setVariant(qVariantFromValue(credentialByte)); Again, the byte is always '\0' in DBus. So don't generate one. This is also justified by the fact that PendingDBusTubeConnection doesn't take it anywhere... copy-pasta from the corresponding code for stream tubes I guess.
From discussion on IRC, it also came out the signals for added/removed contacts might be laid out in a better way, having a signal for an added contact and a signal for a removed one.
Branch updated, with all of the concerns addressed. Lcov is still looking as good as before (and actually even slightly better). Bonus points: added the split signals, and summarized the DBus discussion in the documentation, to help 3rd parties understanding the behavior.
The branch has been force pushed and ported to tp-qt. Instead of making a fix commit, which would have made bisect impossible, I ported commit-by-commit, making sure each commit in the branch made the tree build. Unit tests still pass.
The branch has been merged. Thanks to Andre and George for their reviews.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.