bus: introduce some sd-bus convenience helpers #15331
Conversation
|
I may have been a bit newline-happy here, but it's pretty consistent with the variadic wrappers I made in that regard, just the addition of the assert(dpi) with newlines before and after makes up the difference. Before going and updating all the call sites let's get the name of this struct solidified and land this. BusDPI is kind of annoying as it's not self-explanatory in name, but BusDestinationPathInterface is obnoxious shrug |
|
One minor point is that currently, when using sd-bus you kinda have to look at systemd's source code in order to figure out best practices since the documentation is still lacking. Introducing a private API like this everywhere makes that a little bit harder since you'd have to duplicate it outside of systemd. Again, a minor point but maybe something to consider. The naming is kind of a problem and I can't immediately come up with anything better. One alternative could be to generate shorthand functions instead using a macro, e.g. Anyway, just a suggestion, the current approach will work perfectly as well if we bikeshed a somewhat decent name for BusDPI. |
|
It's not like there's any magic going on here making things opaque.. The naming is a mechanical removal of the sd_ prefix, the wrappers are obviously mapped directly to sd-bus API with nothing more than dereferencing a struct to members for parameters of same name, I don't think it's a valid concern. If the assumption is that everyone will want this API to the extent that they'd all duplicate it, then it's just an argument to add this to sd-bus instead of just making it a private wrapper for systemd... |
|
Example of where this PR takes us as-is: |
There was a problem hiding this comment.
If the assumption is that everyone will want this API to the extent that they'd all duplicate it, then it's just an argument to add this to sd-bus instead of just making it a private wrapper for systemd...
I think we can make that assumption. Systemd codebase clearly shows that this is a very common pattern, and we can assume that other users programs will show the same pattern.
Also, there's "prior art" with the _cleanup_ macros. We made them public to allow writing code with the best modern syntax without pointless duplication in dependent projects.
I would like the patch to make those new functions public.
Also: the structures should be defined as static const.
src/shared/bus-util.c
Outdated
| int bus_call_method_async( | ||
| sd_bus *bus, | ||
| sd_bus_slot **slot, | ||
| const BusDPI *dpi, |
There was a problem hiding this comment.
Maybe call this "sd_bus_address" ?
src/shared/bus-util.h
Outdated
| typedef struct BusDPI { | ||
| char *destination; | ||
| char *path; | ||
| char *interface; |
keszybz
added
reviewed/needs-rework 🔨
|
@keszybz Then what do we call this flavor of the functions? As a private API it's easy, I just removed the 'sd_' prefix and we're good. I can't just put back the 'sd_' prefix as those functions are already taken, and I presume we don't want to break the API of those? So what naming scheme to use in the public sd-bus incarnation? |
|
|
|
Sigh, so instead of shortening the name we'd be making it eight chars longer... on helpers which part of the primary purpose in my mind is to shrink their sizable call site footprint. Part of why I liked the private bus-util.[ch] approach was the convenient name shortening that happened to be consistent with in-tree precedence. A lot of the call sites have been able to just barely become reasonable length flattened to a single line. |
|
I'm leaning more towards a private API as well to keep things concise. One more observation I made was that almost all of the existing D-Bus APIs follow the same naming pattern:
If we just make the convenience methods take <name> as an argument, we can get away with something like this: r = manager_call_method(bus, "home", "ListHomes", &error, &reply, NULL);From "home", we can derive the destination, object and interface. We would have to have special handling in the implementation for those few APIs that don't suffix their interface with ".Manager" but that can be easily hidden in the implementation and new daemons will very likely follow the same naming scheme already used everywhere else (see homed). Of course, this is very tightly coupled to a specific naming scheme but maybe we should just (ab)use the consistency that's already there to our advantage. This doesn't take away that an address based public API would still be nice to have, but even if we had that, we'd probably still want a wrapper around it if the goal is to have really concise D-Bus calls since it's necessarily more generic and more verbose because it's a public API. |
|
An idea: instead of a structure, use a nulstr for the three strings? This will make the definitions a bit shorter. |
|
yuck, I was already thinking I should add designated initializers in the home-busdpi exploration branch to give names to the strings defining the _mgr objects, catering to @DaanDeMeyer's point of this code often serving as a guide to those using sd-bus. Edit: added and pushed to home-busdpi, clearer without more lines, I think it's OK. I'm just not sold on BusDPI, otherwise I do think it makes sense to have this private under bus-util, for now anyways, it's harmless to land privately to get our code concise - there's always time to make it public and quibble over what names and API that form should take. That's the beauty of private vs. public code, public is such a long-term commitment. |
|
What about leave private for now, but s/BusDPI/BusAddress/? |
keszybz
removed
the
reviewed/needs-rework 🔨
WFM. We can always make it public later. |
Many of the convenience functions from sd-bus operate on verbose sets of discrete strings for destination/path/interface/member. For most callers, destination/path/interface are uniform, and just the member is distinct. This commit introduces a new struct encapsulating the destination/path/interface pointers called BusAddress, and wrapper functions which take a BusAddress* instead of three strings, and just pass the encapsulated strings on to the sd-bus convenience functions. Future commits will update call sites to use these helpers throwing out a bunch of repetitious destination/path/interface strings littered throughout the codebase, replacing them with some appropriately named static structs passed by pointer to these new helpers.
|
hmm, maybe the structure should be renamed. "BusAddress" sounds like it was encapsulating a reference to a bus itself. But what it actually does is being a reference to some interface on some object on some service (without specifying any particular bus). I think |
|
The docs use "bus object" to refer to "sd_bus *". |
|
ah, christ... "bus object" is indeed confusing, could be a bus object, or a bus object, noone knows ;-) maybe BusShortcut, BusReference or so? |
|
+1 for "BusShortcut". |
|
BusLocator? BusTarget? BusDestination? |
|
I like BusLocator I must say |
|
Yeah, BusLocator has nice ring to it. |
Mechanical rename in response to systemd#15331 (comment)
|
BusLocator it is #15382 |
Mechanical rename in response to #15331 (comment)
Mechanical rename in response to systemd/systemd#15331 (comment)
Many of the convenience functions from sd-bus operate on verbose
sets of discrete strings for destination/path/interface/member.
For most callers, destination/path/interface are uniform, and just the
member is distinct.
This commit introduces a new struct encapsulating the
destination/path/interface pointers called BusAddress, and wrapper
functions which take a BusAddress* instead of three strings, and just pass
the encapsulated strings on to the sd-bus convenience functions.
Future commits will update call sites to use these helpers throwing
out a bunch of repetitious destination/path/interface strings littered
throughout the codebase, replacing them with some appropriately named
static structs passed by pointer to these new helpers.