Issues with new D-Bus API

[Witold Sowa]

Hi Marcel,

Marcel Holtmann pisze:
> Hi guys,
> 
> so I have been hacking on a client using the new D-Bus API and found a
> couple of things that are sort of weird and could be done way simpler.
> Some of them seems just copied from the old API and I think it is time
> to fix this now before we actual get real users of this API.
> 
I started to work on some patches to address some of those issues. You
can pull it from my git: git://repo.or.cz/hostap-gosc2009.git dbus
branch. I haven't tested it yet and I haven't modified python test scrip
to fit API changes too, but the documentation is up to date. There is
some duplicated code cleaning too.

> 1) DebugParams parameter as a struct is too complex
> 
> The specification describes this as debugging properties. The structure
> elements are: debug level (i), show timestamps (b), show keys (b).
> 
> This is just again properties of properties and a bit too complex. I do
> prefer if I can use tools like d-feet and just go in and change on value
> here instead of dealing with D-Bus structs.
> 
> So my proposal would be to use "DebugLevel", "DebugShowTimestamps" and
> "DebugShowKeys" native properties instead. In addition this allows to
> switch timestamps on/off without reading out the whole debug struct
> first.
> 
Agreed and done.

> 2) The InterfaceAdded signal is actually InterfaceCreated
> 
> The specification talks about the InterfaceAdded signal and that is how
> it should be. Even if the actual method is called CreateInterface. The
> current codes emits InterfaceCreated signal. It propose fixing the code
> to match up the specification and be in sync other signals like BSSAdded
> and NetworkAdded.
> 
Of course, it was a mistake. Changed to InterfaceAdded.

> 3) CreateInterface properties are weird
> 
> The specification list the "Bridge_ifname" property. That should be
> called "BridgeIfname" to match the name of the interface properties.
> 
Where exactly did you find "Bridge_ifname" property name?

> In addition I prefer if we can have "Ifindex" property and only "Ifname"
> or "Ifindex are required to be provided. Same goes for including the
> "Ifname" property for the interface.
> 
> Having the interface name is nice, but it is not guaranteed to be
> stable. I looked through the wpa_supplicant code and it doesn't really
> expose the index, but the nl80211 driver does. I am worried about
> interface renames while WiFi operation is in place. The only stable
> identifier is the index in Linux.
> 
Well, Dbus interface is just frontend. We can't expose variables which
are not available from wpa_supplicant (i.e. we can't get properties
values directly from driver). The other issue is that wpa_supplicant is
supposed to work with other drivers than nl80211.

> And just supporting both, "Ifname" and "Ifindex", doesn't bloat the API
> at all. It actually does make it easier for the client.
> 
> 4) Changes to Scanning property
> 
> The Scanning property of an interface only changes if wpa_supplicant
> triggers the scanning. Not if you trigger it via iw or other tools. The
> kernel emits a signal when a scanning is started. So at least with the
> nl80211 we should be using that one and change Scanning property to
> match the actual scanning state of the device.
> 
Ditto.

> 5) StateChanged signal is pointless
> 
> The StateChanged signal is pretty much pointless. I would actually
> prefer if we just send a PropertiesChanged signal with the State
> property instead. I know that we potentially loose the old state, but
> that is not a problem since that is available via the property in the
> first place. And every client needs to track all state changes anyway.
> So having the old state in that signal is at no real benefit. It just
> makes the API uneven and the clients more complex.
> 
Agreed. I suppose that the motivation for this was that state changed
handler in NM takes old and new states as arguments. Interesting thing
is that it doesn't make any use of the old_state.

> 6) Usefulness of ScanDone signal
> 
> The ScanDone signal is pretty nice to have, but currently it only shows
> if the scan succeeded or failed. What about actually including the
> information from the nl80211 driver on why the scan was triggered?
> 
And again. Is that information available directly from scan function or
we would need to get if somehow from driver's private data?

> An alternative would be to make the Scan method actually return the
> result. Especially with the BSSAdded and BSSRemoved doing all the work,
> that might be a way better alternative. Essentially the client only
> cares about the results of scans it triggered anyway.
> 
But then the result of scan method would arrive to client quite long
after calling. I would prefer to not have any "blocking" methods.

> 7) Change of BSS properties
> 
> As mentioned earlier the BSS properties should be native properties and
> not properties of properties.
> 
Done,

> In addition we should be removing "Quality" and "Noise" since they are
> either pointless or the kernel is not exporting them anyway.
> 
> Rename "Level" into "Signal" to match what iw is showing. Does anything
> using "RSSI" for this would be better? And lets make this a int16
> instead of using a int32. That is legacy WEXT crap.
> 
done,

> The "Frequency" property should be uint16 instead of int32. I really
> have no idea why it is int32. That is just pointless.
> 
done,

> Remove "Capabilities" and replace it with "Mode" as string. The valid
> modes should match the interface capabilities. This makes it nicely
> consistent. Also use "Privacy" as boolean to indicate encryption.
> 
and done :-)

> Is anybody really using "MaxRate" property. Wouldn't it better to export
> "Rates" as array of uint16? And if it is sorted the client could easily
> extract the max rate from there.
> 
Agreed. We won't lose any information and even give more. I changed
MaxRate type to uint16, but the array will be better.

> 8) Network properties
> 
> That thing is still a mess and we should do something about it. Having
> everything as strings is kinda bad. I do remember we talked about it,

Heh, I do remember it too. IIRC, Dan wanted to have all options as
strings, but I don't remember why :)

> but not being able to clearly match up a SSID from a network with a SSID
> from a BSS. I do think we should have something cleaner and simpler
> there and not necessary copy the syntax of the wpa_supplicant
> configuration file. Especially the interface capabilities like KeyMgmt
> should match the property names we use in the network configuration.
> 
We could do some mapping from config options to properties.
Additionally, we could keep a dictionary with all options in strings too
if it is really needed.

> Also there should be an easy way to change a property later one. Like
> for example change the "Priority". Then we can have multiple network
> configuration and modify them at runtime. And don't need to remove and
> re-add them.
> 
> 9) Miscellaneous
> 
> I already mentioned that we should add the full dictionary of properties
> as second argument to the signals InterfaceAdded, NetworkAdded and
> BSSAdded. I do think that we can leave BlobAdded out of this since it
> just is a data storage.
> 
Done, but still I'm not sure if it's right. Those signals are much
bigger now and it probably may have some negative impact on performance.
If a client is interested in object properties, it can always ask about
those via GetAll.

10) All BSSs properties at once.

As you proposed earlier, a method returning a dict with BSS objects and
its properties would be quite useful for clients connecting to running
wpa_supplicant. I'm not sure if DBus allows to make dictionaries with
other keys than strings. In case it doesn't, we can store object paths
as strings in dictionary keys or we can return an array of structs
(pairs) like a(oa{sv}). Similar methods would be useful for networks and
 interfaces.

Regards,
Witek