Hi Jose, 

I applied the changes to my local source code(based on connman 1.29) for testing, and got some issue.
When trying to establish p2p connection for using Miracast, it's failed.
Though, the issue has gone with the below change.
I'm not sure if this issue occurs only on connman 1.29, but would like to share this issue to you. 

--- a/connman/plugins/wifi.c
+++ b/connman/plugins/wifi.c
@@ -3183,7 +3183,7 @@ static void wps_event(GSupplicantInterface *interface,
        struct wifi_data *wifi = g_supplicant_interface_get_data(interface);
 
        /* Do nothing here for P2P Connections */
-       if (wifi->p2p_connecting)
+       if (is_p2p_connecting())
                return;
 
        switch (state) {

And it looks like every dbus_malloc0 needs to check if it's NULL and return -ENOMEM. :)

Regards,
Eunok

2016-08-29 23:57 GMT+09:00 Jose Blanquicet <blanquicet@gmail.com>:
Hi Patrik,

thanks for your feedback.

>> Two D-Bus methods were added to Technology Interface:
>>       - void Start_AP_WPS(string authentication)
>>       - void Start_STA_WPS(string authentication)
>
> I'm trying to wrap my brains around the functionality added here. AFAIK
> having these two functions in the technology API makes ConnMan able to
> pass WiFi Alliance testing, right? And you also propose this for
> regular use? But then the precedence between StartSTAWPS() and already
> connected services are not that clear cut.
>

Yes, with these two function ConnMan should be able to get
certificated on WPS by WiFi Alliance.
Yes, the idea is to deprecate the current implementation.
The precedence is explained below.

> Drop '_' from method names.

The idea would be to merge them into a unique one as Tomasz suggested:
StartWPS(type, auth). Where type could be "ap" or "sta". Please look
an important comment regarding this below.

>> With Start_STA_WPS method, WPS was completely detached from Service
>> and now ConnMan follows WiFi Alliance specifications. This patch
>> keeps current WPS implementation for compatibility support, but as
>> deprecated.
>
> As far as I understand the above correctly, the point of this is to
> have ConnMan the STA to "push the WPS button" first, right? And with
> connman requesting WPS, the first AP to also enable WPS will then get
> selected.

Exactly, the main problem of the current implementation is the fact
that user must select the AP to connect to when he/she starts WPS,
which violates the WiFi Alliance specification.

> And the code for (manually) selecting a service which happens to be
> broadcasting/announcing WPS, will automatically of course be connected
> using WPS as before.

Yes, as I mentioned, the patch keeps current WPS implementation but as
deprecated. I didn't duplicate the code, at certain point both
implementations converge to the same wpa_supplicant method call,
therefore they share the code from that point forward.

>> In case of multiple interfaces, Start_STA_WPS will prioritize the
>> STA-only interfaces if they exist, otherwise it will try on STA/AP
>> ones and only as last option it will use STA/AP/P2P interfaces.
>
> If the STA/AP/P2P capability detection works, then yes. To be sure, AP
> interfaces were marked such only after successfully setting up an AP
> for tethering as the capabilities haven't been that perfectly provided
> by wpa_supplicant in the past.

Yes, I used the flag wifi->ap_supported to confirm the AP capabilities
reported by wpa_supplicant. If this flag indicates that AP is not
actually supported then this interface is considered as a STA-only. As
you know this flag is set only after trying to set up an AP for
tethering, but if the user has not tried tethering yet and AP is
present in the capabilities reported by wpa_supplicant then I have
faith on those capabilities and that interface will be taken into
account only if the STA-only interfaces are not available (If they
exist).

>>  If the selected interface is already connected to a service or p2p
>> device, it will be disconnected and then the WPS Session will start
>> on it.
>
> Hmm, disconnecting a connected interface sounds a bit unfortunate. Is
> this encouraged by the WiFi Alliance specification? If yes, we might
> not want not to do that, the STA or P2P connection in use can be more
> important for the user than the opposite. From a ConnMan point of view
> it is ok to return an -EBUSY equivalent over D-Bus if everything is
> already in use. Opinions anyone?

No, it is not specified. We just wanted to avoid user to have to
manually make a disconnection. However, for us it is not a problem to
return -EBUSY if it is preferred.

>>  Interfaces in AP mode (Tethering enabled) will not be taken into
>> account, they will be skipped.
>
> Good.
>
>> Start_STA_WPS method will finished successfully when WPS-Credentials
>> are correctly received. From that point, all the notifications(error
>> or success) will be sent through the service corresponding to the
>> received credentials.
>
> Shouldn't StartSTAWPS() return the service path on success or how else
> is it known which service got connected via WPS? We do want to provide
> the user with complete information on what the end result was.

Yes, you are right. A "problem" could be if we merge both calls into
an unique one StartWPS(type, auth) as Tomasz suggested, because for
type "STA" the method will return the service path but for "AP" the
return will be an empty string "", is it fine for both of you?

In addition, this patch is based on wpa_supplicant implementation thus
StartSTAWPS() replies successfully when WPS-Provisioning is finished
(Event: WPS Success) not when the whole connection is completely
finished. At this point the WPA2 phase will automatically start using
the passphrase just gotten in the WPS phase and then using the service
path, the user will be able to get all the signals generated in this
second phase on that service, for instance the changes on the "state"
property "association"=>"configuration"=>"ready", as well as the
information related to the IP and the possible errors in this phase.

>> In addition, by using Start_AP_WPS method, ConnMan now supports WPS
>> in AP mode (As registrar) which allows external devices get connect
>> by using WPS. For this method, if tethering is enabled it will start
>> a WPS Session on the corresponding tethering interface, otherwise it
>> will reply with a "PermissionDenied" error or "NotSupported" error if
>> the interface has tethering enabled but it does not support WPS.
>
> This leaves a time window when tethering is enabled using a passphrase,
> but WPS is not active, which was the intention? Else one needs to have
> an attribute for WiFi that can be set before tethering is enabled.

An external STA will be able to get connected at any time when we are
tethering either using the passphrase or WPS (If it has been started).
In fact, in the WPS phase the AP will provide to the external STA the
passphrase to be used, which is the same used to set the tethering in
ConnMan (That is the scope of WPS). Once the external STA gets
connected for the first time it will always use that passphrase for
future re-connections, we tested it. Sorry but what is exactly the
problem here?

>> Technology documentation was updated.
>>
>>
>> ---
>>  doc/agent-api.txt         |   6 +
>>  doc/technology-api.txt    |  35 +++++
>
> Although small, these two should go into two different patches as they
> document things.
>>  gsupplicant/gsupplicant.h |  15 +++
>>  gsupplicant/supplicant.c  | 272 +++++++++++++++++++++++++-----------
>> ---
>
> Separate patch for the basic work on the low level supplicant methods.
>>  include/technology.h      |  10 ++
>>  plugins/wifi.c            | 322
>> +++++++++++++++++++++++++++++++++++++++++++---
>>  src/connman.h             |   1 +
>>  src/peer.c                |   5 +-
>>  src/technology.c          | 119 +++++++++++++++++
>
>
> From here I'd single out the patches in plugins/wifi.c and src/peer.c
> from the ones in technology. I hope plugins/wifi.c then handles all of
> the WPS STA/peer things for the old and new cases with the same code.

Even if they have no sense on their own? I would try to see if this
patch can be split as following:

1. gsupplicant: Add support for WPS specific methods
(gsupplicant/gsupplicant.h and gsupplicant/supplicant.c)
2.  wifi: Add support for WPS specific methods (plugins/wifi.c)
3. peer: Export function to check WPS PIN (src/peer.c and src/connman.h)
4. technology: Add specific WPS D-Bus methods (include/technology.h
and src/technology.c)
5. doc: Add documentation for new WPS D-Bus methods (doc/agent-api.txt
and doc/technology-api.txt)

On the other hand, as I mentioned before the old and the new WPS
implementation converge to the same wpa_supplicant method call, so
they share some pieces of code in plugins/wifi.c. I tried to pay
attention to not duplicate code, as I did for example with the
function for check WPS PIN in src/peer.c.


Regards,

Jose Blanquicet
_______________________________________________
connman mailing list
connman@lists.01.org
https://lists.01.org/mailman/listinfo/connman