New D-Bus API #175
New D-Bus API #175
Conversation
ghost
mentioned this pull request
|
Ping @lathiat |
|
nice work @Simoninator ! Looking forward to see this getting merged. |
|
Hey @lathiat, |
|
Hey @lathiat, |
|
Planning to get on top of it this week |
|
Probably this week? |
1 similar comment
|
Probably this week? |
|
Any news about this? |
|
Well, I think that this pull request is never going to be merged! |
|
@lathiat, any news on merging this PR? |
|
@lathiat could you find some time to have a look at this PR and maybe merge it? |
|
I'd much prefer if we avoid tacking a "2" on the end of the function names (e.g. ServiceBrowserNew2). I've got a couple of ideas on how we can do that The first thing that makes sense would be to use a versioned API. The following documents cover this topic: Initially I got stuck on this point "Note, however, that supporting multiple interface versions simultaneously requires that object paths are versioned as well, so objects must not be put on the bus at the root path (‘/’). This is for technical reasons: signals sent from a D-Bus service have the originating service name overwritten by its unique name (e.g. com.example.MyService2 is overwritten by :1:15)." I think? we can get away with this for our limited use case, because the interface name is always sent when calling ServiceBrowserNew and we don't need to persist what interface was used later. So at least for this specific change we can get away with it. (for the below I specifically reference ServiceBrowserNew since that's the most common however obviously this also applies to a number of other Browsers and Resolvers so assume when I mention ServiceBrowser below that I am talking about all of them - I just found it helpful to give an explicit example) If it's too difficult or otherwise problematic to user the interface version; I think instead we can simply change the name instead. Instead of "ServiceBrowserNew2" we can use "ServiceBrowserPrepare" which then requires an explicit call to "Start" where as "ServiceBrowserNew" would retain it's existing behavior and automatically Start as part of the creation. Besides looking less silly having "2" in the new name used permanently, it also more clearly distinguishes the difference in behavior between the two. The only slightly problem is that clients supporting the new and old server APIs would need to handle calling Prepare and then falling back if it gets back a NoMethodError. Lastly I considered how we can fix existing clients alongside providing a new v2 API for this. We should be able to defer the start of the resolvers on the server-side for a short time to give the client time to setup it's signal receiver. This should make clients that are currently likely to fail, less likely to fail. I did a few quick tests and the best case delay for the signal handlers to be registered seems to be around 0.6 milliseconds so I don't think we'd need to delay it by much - only a few ms to handle most cases. I'd probably want to re-test this on a loaded and slower system to make sure it's not orders of magnitudes out. I think we should do this regardless of whatever other API change we end up making. If we're worried about issues caused by the reply delays (though if we only need a few ms I am not too worried about it - if we ended up needing 100s of ms it might be more of a concern) - we could probably only do the delay for the Browsers and skip it for the Resolvers since we have directly-reply dbus methods for ServiceResolve/HostNameResolve/AddressResolve and don't need registering to a new object path. We could also obviously just skip the delay when the new version API is used and/or when Start is called - just cancel the delay and start anyway. Which brings me full circle to what I think might be the most ideal and backwards compatible solution (1) Create a new Avahi server interface API version, org.freedesktop.Avahi2
Lastly we have a number of avahi-core users outside of avahi-daemon and the current change will silently break those apps in that the API isn't backwards compatible. That's probably not necessary -- similar to the first suggestion above we could add a new _prepare method to go with _start and keep _new with the existing behavior to just call _prepare and then _start. Even if we don't add _prepare to the client/dbus API. Would love feedback/discussion on each of the ideas. |
and incremented the API Version
two dbus avahi server interfaces
|
Thank you @lathiat for your response. I also think a second interface is the best option we have, as it doesn't break existing behaviour. In my opinion, I think we should change the method names from xBrowserNew to xBrowserPrepare in the new interface. The API change would then be recognized directly in the new dbus interface and there might be no confusion. I haven't found the time to look for a suitable timer solution to delay the replies in the old interface. Is there an implementation of a timer available, that can be used to delay the callbacks in the event/poll loop? |
…rs after their preparation
|
@lathiat ping |
|
@lathiat Could you please have a look at the latest commits of this pull request? |
|
@lathiat Could you please have a look at the latest commits of this pull request? |
|
As I read the current pull request, the general approach is
This all looks good to me, I will check a couple extra details and do some testing next week but should more or less be OK. |
|
Will need to bump the minor version numbers for the libraries in configure.ac but I can take care of that. In theory since we keep the old New function we shouldn't have to bump the major number. |
|
@lathiat did you find some time to check the PR? |
Fix NULL pointer crashes such as when doing "ping .local.", originially introduced in #175 (CVE-2021-3502) Closes: #338
Looks like the policy wasn't updated when the Server2 interface was introduced. It's a follow-up to avahi#175
This pull request implements the API change mentioned in #173