[PATCH] more dbus control interface functionality

[Dan Williams]

On Wed, 2006-06-21 at 20:36 -0700, Jouni Malinen wrote:
> 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?

I can take a stab at cleaning them up, based on your comments.  You've
probably got better things to do :)

> > 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..

Not a whole bunch, I think, so you're probably right here.  The main
missing item from the dict helper code is arrays of strings, but that's
not so hard to add twice.  We need to strike a balance here between the
raw dbus API and the one presented by the helper code; can't make it too
complex or we might just as well use the dbus API directly.

So; I will re-style the dbus-dict-helper.* files for wpa_supplicant,
including documentation comments.

> 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?

Any code submitted by me to this list would have my (implicit) sign-off
that it's available under the dual license GPL + BSD.  Since I'll be
re-styling the wpa_supplicant versions of these files, there won't be
any direct code drops of them from the NM versions.  Were I to update
the files with code other than mine, I would request approval from the
original committed for a dual-license addition before submitting the
patches to this list.

> 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.

Agreed; this happens every now and again.  However, as above I think the
dbus people aren't trying to create too many higher-level APIs so as to
avoid API-creep, retain flexibility, and keep the number entry points
low.  Dict types were a recent addition (somewhere around 0.3 or 0.4 I
think) so there hasn't been a ton of time for them to filter out into
widespread use yet.  They are mostly for language bindings (like Python,
glib, Qt, etc for hash tables generally).

> 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..

Yes, dbus messages can also be errors/exceptions.  If I perform a method
call on your dbus object, and an error occurs in your object while
handling that method call, you can throw an error that I may handle.

dbus errors are _not_ exceptions in the traditional C++/Python sense.
They are simply DBusMessage objects with an "error" type, no different
than a method call or a method return.  Language bindings may do with
them what they want; Python converts them to standard python exceptions
(as you see in the example I provide), C++ & Java bindings may convert
them to exceptions, but in plain C it is up to the caller to handle
them.

When you make a method call you do:

reply = dbus_connection_send_with_reply_and_block (con, message,
<timeout>, &error);

Here, the &error is a local error variable if an error occurs before the
message is sent.  But the 'reply' argument may be the actual method
return, or it can be an error.  You check with:

if (dbus_message_is_error (reply)) {
    <handle the error case>
} else {
    <proceed normally>
}

It's no different than the wpa_supplicant control sockets returning
something other than "OK".  Perhaps my choice of wording there was bad.

> 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

Will fix.

> 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?

Hmm, I tried to kill all of those in the ctrl_iface_dbus* files.  When
restyling the dbus-dict-handler* files I'll fix them up as well.

> 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)

Will fix.

> 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.

Quite; I'm going to need to add more than a few callbacks like this for
the dbus stuff, and I'm sure the normal socket-based control interface
would like to know about the same events.

So for now I'll leave this as-is, but going forward lets identify all
the place that need events and work out a solution for them.

Thanks,
Dan