Re: [PATCH 3/4] wsc: Refactor to make the DBus interface reusable

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3968 bytes --]

Hi Andrew,

<snip>

> The disconnect is handled at a lower level, by wsc_cancel.
> wsc_disconnect_cb doesn't need to do anything related to the internal
> protocol state.
> 
> The part 1. exposes an API that is used by 3. in wsc.c, or by the
> external caller which also supplies a callback like wsc_disconnect_cb.
> That callback also can't mess with variables like wsc->wsc_association
> (sitting outside wsc.c it has no way anyway).
> 

Okay, so I had to stare at this again for a while and I think I get it 
now.  Note that you only call wsc_connect_cleanup if netdev_disconnect 
fails.  So wsc_connect_cleanup is not called unnecessarily.

<snip>

> 
> In this case wsc->pending was only part of 3. (out of the three state
> machines in wsc.c I mentioned) and p2p.c had it's own "pending" which
> is also used as a flag internally.  I guess it makes sense to change
> this to handle the dbus message completely in wsc.c so p2p.c doesn't
> have to see the actual messages.

If it was me doing it, I'd try to unify the D-Bus API handling aspects. 
Especially since you went to the trouble of abstracting all the operations.

> 
>>
>> If you want to re-invent this inside p2p, then there's really no need
>> for this wsc_ops abstraction.  Just have something like:
>>
>> struct wsc_dbus_object {
>>          bool station;
>>          union {
>>                  struct wsc_p2p *p2p;
>>                  struct wsc *wsc;
>>          }
>> }
> 
> I don't follow.  Our entry points are the DBus method handlers, which
> are in wsc.c.  How will the control pass from wsc.c to p2p.c (or to
> the station specific CBs) without the wsc_ops?

it would just invoke the relevant function calls directly.  i.e.

wsc_p2p_push_button(p2p, msg);
wsc_p2p_start_pin(p2p, msg);
wsc_p2p_cancel(p2p, msg);

That would give you a whole lot more freedom to do whatever in p2p.

> 
> I think I did (this was a month ago) ;-)  You're right, the idea is
> for the user_data to be the wsc object itself because I didn't split
> the state into separate structs.  So in the station mode case
> wsc_add_interface calls wsc_new, then overwrites wsc->user_data.

Yep, I think I missed the part of you overwriting the user_data.  You 
went through all this trouble of defining an API for setting it and then 
turn around and backdoor it ;)  That is what threw me off here and also 
in the disconnect_cb bits above.  If user_data was meant to be set 
externally, it would have been clearer to add wsc_set_userdata() method 
or have the caller pass in the cb/userdata into the operation directly.

> 
> .connect and .cancel are both being utilized, and .enrollee_done_cb is
> also being utilized but at a different level.  There's currently one
> user_data for them.

So while this works, I would think that separating the layers more 
explicitly would make things more elegant.  You have the EAP-WSC state 
machine object that takes care of the actual WSC connection:
	wsc_enrollee_new
	wsc_enrollee_start
	wsc_enrollee_cancel
	wsc_enrollee_free

Then you have the p2p/station layers that take care of scanning, etc. 
This is where your ops stuff can live.  Something like:

struct wsc {
	const char *(*get_path)(struct wsc *wsc);
	int (*connect)(struct wsc *wsc, const char *pin,
			connect_cb_t cb, void *userdata);
	int (*cancel)(struct wsc *wsc, cancel_cb_t cb, void *userdata);
	void (*remove)(struct wsc *wsc);
};

Then for p2p you can do:

struct wsc_p2p {
	// some members
	struct wsc super;
};

struct wsc *wsc_p2p_new();

for station:
struct wsc_station {
	struct wsc super;
	// some members;
	struct station *station;
	uint32_t station_state_watch;
	...
};

struct wsc *wsc_station_new();

And if you want to unify the D-Bus bits, then:

struct wsc_dbus {
	struct wsc *wsc;
	struct l_dbus_message *pending;
	struct l_dbus_message *pending_cancel;
};
	
Regards,
-Denis