[PATCH] more dbus control interface functionality

[Jouni Malinen]

On Sun, Jun 18, 2006 at 12:52:15AM -0400, Dan Williams wrote:

> Continue to fill out the functionality of the dbus control interface.

Thanks!

> This patch adds:
> 
> - Interface addition and removal
> - Document most functions
> - Scan requests
> - Scan result signals
> - Property retrieval for APs
> - dbus config file for wpa supplicant

In general, this looks good and I could probably add it after some
coding style cleanup. Do you prefer to get more comments on what areas
should be changed and doing the changes yourself or me just doing the
changes while committing these?

> The dbus-dict-helpers.c and .h files are copied over from NetworkManager
> public API, and therefore have the 'nmu_' function prefix, doxygen style
> documentation, a slightly different code style, and the '-' character in
> the file name.  I'd really like to keep these files as close to the NM
> versions as possible so that any fixes/additions to the NM ones go to
> wpa_supplicant with a minimum of rework.  But if any of these issues are
> a problem, I suppose we can work something out.

How many fixes/additions do you think we could expect on these files? I
have a preference of using the same coding style throughout
wpa_supplicant implementation and including some files in different
style would be somewhat of an unfortunate example which I would not like
others to copy..

Another potential issue would be in the area of licensing. If I've
understood correctly, the version of these files in NM are licensed
under GPL and I would expect all future contribution to NM to be under
GPL. However, wpa_supplicant requires that BSD license is allowed as an
alternative. It looks like you have committed all the changes into NM
for these two files, so I would assume you/RedHat can re-license them.
However, what if someone else where to modify these files? Can we assume
that these contributions will be also under BSD or would contributions
from others need to be kept away from wpa_supplicant versions?

By the way, if this is general code that would be useful for number of
applications, should it be available from a dbus library or is there
some reason to have this in NM (and then copy these to whatever programs
need to interract with NM over dbus)? I'm not very familiar with dbus,
so I may be missing something here.



I haven't yet went through all the details, but here are some random
comments:

I got a bit scared when I noticed this comment in
wpas_dispatch_iface_method:

"Returns: a reply dbus message, or throws a dbus error"

What exactly is that "throws a dbus error" referring to? Throwing errors
and exception handling sounds bit too much like C++/Java to me ;-), but
I hope this is actually referring to something else.
new_invalid_*_error() functions seemed to return an error messages..



wpa_supplicant_dbus_notify_scan_results:

	path = wpa_zalloc(100);
	snprintf(path, 99, WPAS_INTERFACES_DBUS_PATH "/%s", wpa_s->ifname);

- wpa_zalloc can fail, so much verify that path != NULL
- snprintf enforces that nul termination is used, so 99 should be 100


There are number of coding style differences (whitespace between
function name and open parantheris, too long lines, etc.). Do you have
local changes on top of these files or can I do some cleanup without
causing too much extra work with it?


ctrl_iface_dbus_handlers.c:

- I would prefer to see some dbus-related prefix etc. on global
  functions (e.g., new_invalid_iface_error() sounds quite generic and I
  would have no idea that would be related to dbus from just the name)


events.c: It might be worthwhile to add a more generic mechanism for
registering callback functions for scan results notifications. This is
already used in number of different areas in wpa_supplicant and a
cleaner solution would be nice to avoid direct calls to a dbus-specific
function from events.c. Anyway, since this mechanism is not here now,
I'm okay with the direct call for now; it just may end up being changed
in the future to be something that dbus ctrl_iface can use to register a
notification callback.

-- 
Jouni Malinen                                            PGP id EFC895FA