| Message ID | 20140925182410.5270713FECF@ushik.mtv.corp.google.com |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > bounces@lists.shmoo.com] On Behalf Of Dmitry Shmidt > Sent: Wednesday, September 24, 2014 20:39 > To: hostap@lists.shmoo.com > Subject: [PATCH] Android: Skip explicit conf_p2p_dev loading for main > interface > > It will be loaded later for Non-netdev p2p interface. > > Change-Id: Ib53daf22c84f2581b499462dc09a74ccfbac226a > Signed-off-by: Dmitry Shmidt <dimitrysh@google.com> > --- > wpa_supplicant/wpa_supplicant.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/wpa_supplicant/wpa_supplicant.c > b/wpa_supplicant/wpa_supplicant.c index f20bc62..9d21fe0 100644 > --- a/wpa_supplicant/wpa_supplicant.c > +++ b/wpa_supplicant/wpa_supplicant.c > @@ -3509,7 +3509,9 @@ static int wpa_supplicant_init_iface(struct > wpa_supplicant *wpa_s, > > #ifdef CONFIG_P2P > wpa_s->conf_p2p_dev = os_rel2abs_path(iface- > >conf_p2p_dev); > +#ifndef ANDROID_P2P > wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > +#endif I think it would be cleaner to do the reading only if iface->p2p_mgmt is set (happens only in case a dedicated P2P DEVICE interface is added). Regards, Ilan.
On Sat, Sep 27, 2014 at 10:23 PM, Peer, Ilan <ilan.peer@intel.com> wrote: > > > > -----Original Message----- > > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > > bounces@lists.shmoo.com] On Behalf Of Dmitry Shmidt > > Sent: Wednesday, September 24, 2014 20:39 > > To: hostap@lists.shmoo.com > > Subject: [PATCH] Android: Skip explicit conf_p2p_dev loading for main > > interface > > > > It will be loaded later for Non-netdev p2p interface. > > > > Change-Id: Ib53daf22c84f2581b499462dc09a74ccfbac226a > > Signed-off-by: Dmitry Shmidt <dimitrysh@google.com> > > --- > > wpa_supplicant/wpa_supplicant.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/wpa_supplicant/wpa_supplicant.c > > b/wpa_supplicant/wpa_supplicant.c index f20bc62..9d21fe0 100644 > > --- a/wpa_supplicant/wpa_supplicant.c > > +++ b/wpa_supplicant/wpa_supplicant.c > > @@ -3509,7 +3509,9 @@ static int wpa_supplicant_init_iface(struct > > wpa_supplicant *wpa_s, > > > > #ifdef CONFIG_P2P > > wpa_s->conf_p2p_dev = os_rel2abs_path(iface- > > >conf_p2p_dev); > > +#ifndef ANDROID_P2P > > wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > > +#endif > > I think it would be cleaner to do the reading only if iface->p2p_mgmt is > set (happens only in case a dedicated P2P DEVICE interface is added). > 1) iface->p2p_mgmt will be set later. 2) In case p2p has separate Netdev - it will have its own iterface, in case for Non-netdev it will still load this file taking it from parent. Can you please describe the situation when we need to load this file for main interface? Thanks, Dmitry > > Regards, > > Ilan. >
> > diff --git a/wpa_supplicant/wpa_supplicant.c > b/wpa_supplicant/wpa_supplicant.c index f20bc62..9d21fe0 100644 > --- a/wpa_supplicant/wpa_supplicant.c > +++ b/wpa_supplicant/wpa_supplicant.c > @@ -3509,7 +3509,9 @@ static int wpa_supplicant_init_iface(struct > wpa_supplicant *wpa_s, > > #ifdef CONFIG_P2P > wpa_s->conf_p2p_dev = os_rel2abs_path(iface- > >conf_p2p_dev); > +#ifndef ANDROID_P2P > wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > +#endif I think it would be cleaner to do the reading only if iface->p2p_mgmt is set (happens only in case a dedicated P2P DEVICE interface is added). 1) iface->p2p_mgmt will be set later. For the case of Non netdev P2P_DEVICE, iface->mgmt. is set in wpas_p2p_add_p2pdev_interface() and this is done before the flow that calls wpa_supplicant_add_iface() -> wpa_supplicant_init_iface(), so using this state variable, the P2P specific configuration file would be read only for a (non netdev) P2P_DEVICE interface. 2) In case p2p has separate Netdev - it will have its own iterface, in case for Non-netdev it will still load this file taking it from parent. Can you please describe the situation when we need to load this file for main interface? In case that a P2P Device has a separate Netdev, you can simply use the –c to pass the configuration file for the interface with all needed parameters, so there is no need to use the –m switch. In the past Jouni wanted to better handle this by having some global configuration file that will hold global configuration parameters that would be shared between all the interfaces. I tried to prepare some solution for this if (still in Jouni’s queue): http://patchwork.ozlabs.org/patch/332033/ http://patchwork.ozlabs.org/patch/332034/ http://patchwork.ozlabs.org/patch/332035/ Regards, Ilan.
On Sun, Sep 28, 2014 at 11:40 AM, Peer, Ilan <ilan.peer@intel.com> wrote: > > > > > > diff --git a/wpa_supplicant/wpa_supplicant.c > > b/wpa_supplicant/wpa_supplicant.c index f20bc62..9d21fe0 100644 > > --- a/wpa_supplicant/wpa_supplicant.c > > +++ b/wpa_supplicant/wpa_supplicant.c > > @@ -3509,7 +3509,9 @@ static int wpa_supplicant_init_iface(struct > > wpa_supplicant *wpa_s, > > > > #ifdef CONFIG_P2P > > wpa_s->conf_p2p_dev = os_rel2abs_path(iface- > > >conf_p2p_dev); > > +#ifndef ANDROID_P2P > > wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > > +#endif > > I think it would be cleaner to do the reading only if iface->p2p_mgmt is > set (happens only in case a dedicated P2P DEVICE interface is added). > > > > 1) iface->p2p_mgmt will be set later. > > > > For the case of Non netdev P2P_DEVICE, iface->mgmt. is set in > wpas_p2p_add_p2pdev_interface() and this is done before the flow that calls > wpa_supplicant_add_iface() -> wpa_supplicant_init_iface(), so using this > state variable, the P2P specific configuration file would be read only for > a (non netdev) P2P_DEVICE interface. > Please correct me if I am wrong but wpas_p2p_add_p2pdev_interface() is called after wpa_supplicant_add_iface() wpa_s = wpa_supplicant_add_iface(global, &ifaces[i]); if (wpa_s == NULL) { exitcode = -1; break; }#ifdef CONFIG_P2P if (wpa_s->global->p2p == NULL && (wpa_s->drv_flags & WPA_DRIVER_FLAGS_DEDICATED_P2P_DEVICE) && wpas_p2p_add_p2pdev_interface(wpa_s) < 0) exitcode = -1;#endif /* CONFIG_P2P */ It is highly possible I am missing something but I am still can not fidn the case when we need to load -m switch file for main interface. > 2) In case p2p has separate Netdev - it will have its own iterface, in > case for Non-netdev it will still load this file > > taking it from parent. Can you please describe the situation when we need > to load this file for main interface? > > > > In case that a P2P Device has a separate Netdev, you can simply use the –c > to pass the configuration file for the interface with all needed > parameters, so there is no need to use the –m switch. > Right, this is exactly my point. > > > In the past Jouni wanted to better handle this by having some global > configuration file that will hold global configuration parameters that > would be shared between all the interfaces. I tried to prepare some > solution for this if (still in Jouni’s queue): > > > > http://patchwork.ozlabs.org/patch/332033/ > > http://patchwork.ozlabs.org/patch/332034/ > > http://patchwork.ozlabs.org/patch/332035/ > This is probably right thing to do but let's wait for Jouni's response. > > > Regards, > > > > Ilan. > > >
Hi Dmitry, My original reply was bounced by the moderator … so here it is again: You are correct; the –m switch does not currently make much sense in case of netdev interface used for P2P_DEVICE operations (and also there is no need to specify it for the main interface). The first call to wpa_supplicant_add_interface() add the network interface, but in that case iface->p2p_mgmt would not be set, so checking would allow to skip reading the p2p device configuration file. After the netdev interface is created, the code creates an additional interface for the P2P Device management (only if one does not exist and the underlying device supports it). So in this case wpas_p2p_add_p2pdev_interface() is called, and only in this case iface->p2p_mgmt is set. So bottom line, I think that a cleaner fix would be to base the reading on the setting of iface->p2p_mgmt. Regards, Ilan.
On 09/29/14 13:05, Peer, Ilan wrote: > Hi Dmitry, > > My original reply was bounced by the moderator … so here it is again: > > You are correct; the –m switch does not currently make much sense in > case of netdev interface used for P2P_DEVICE operations (and also there > is no need to specify it for the main interface). > > The first call to wpa_supplicant_add_interface() add the network > interface, but in that case iface->p2p_mgmt would not be set, so > checking would allow to skip reading the p2p device configuration file. > > After the netdev interface is created, the code creates an additional > interface for the P2P Device management (only if one does not exist and > the underlying device supports it). So in this case > wpas_p2p_add_p2pdev_interface() is called, and only in this case > iface->p2p_mgmt is set. > > So bottom line, I think that a cleaner fix would be to base the reading > on the setting of iface->p2p_mgmt. Related to this I created a patch that would make the -m option redundant. This is done by skipping networks and ignore 'p2p_disabled=1' in the configuration when iface->p2p_mgmt is set. Regards, Arend > Regards, > > Ilan. > > > > _______________________________________________ > HostAP mailing list > HostAP@lists.shmoo.com > http://lists.shmoo.com/mailman/listinfo/hostap
> -----Original Message----- > From: Arend van Spriel [mailto:arend@broadcom.com] > Sent: Monday, September 29, 2014 16:36 > To: Peer, Ilan > Cc: Dmitry Shmidt; hostap@lists.shmoo.com > Subject: Re: [PATCH] Android: Skip explicit conf_p2p_dev loading for main > interface > > On 09/29/14 13:05, Peer, Ilan wrote: > > Hi Dmitry, > > > > My original reply was bounced by the moderator … so here it is again: > > > > You are correct; the –m switch does not currently make much sense in > > case of netdev interface used for P2P_DEVICE operations (and also > > there is no need to specify it for the main interface). > > > > The first call to wpa_supplicant_add_interface() add the network > > interface, but in that case iface->p2p_mgmt would not be set, so > > checking would allow to skip reading the p2p device configuration file. > > > > After the netdev interface is created, the code creates an additional > > interface for the P2P Device management (only if one does not exist > > and the underlying device supports it). So in this case > > wpas_p2p_add_p2pdev_interface() is called, and only in this case > > iface->p2p_mgmt is set. > > > > So bottom line, I think that a cleaner fix would be to base the > > reading on the setting of iface->p2p_mgmt. > > Related to this I created a patch that would make the -m option redundant. > This is done by skipping networks and ignore 'p2p_disabled=1' > in the configuration when iface->p2p_mgmt is set. > Does this handle the case of update_config=1? In this case, if the configuration file is shared between 2 different interfaces, one of them will override the configuration of the other when wpa_supplicant exists or instructed to save the config. Regardless, I think that the approach of have one global configuration file to hold device wide configuration is cleaner and more scalable. Regards, Ilan.
On 09/29/14 15:52, Peer, Ilan wrote: > > >> -----Original Message----- >> From: Arend van Spriel [mailto:arend@broadcom.com] >> Sent: Monday, September 29, 2014 16:36 >> To: Peer, Ilan >> Cc: Dmitry Shmidt; hostap@lists.shmoo.com >> Subject: Re: [PATCH] Android: Skip explicit conf_p2p_dev loading for main >> interface >> >> On 09/29/14 13:05, Peer, Ilan wrote: >>> Hi Dmitry, >>> >>> My original reply was bounced by the moderator … so here it is again: >>> >>> You are correct; the –m switch does not currently make much sense in >>> case of netdev interface used for P2P_DEVICE operations (and also >>> there is no need to specify it for the main interface). >>> >>> The first call to wpa_supplicant_add_interface() add the network >>> interface, but in that case iface->p2p_mgmt would not be set, so >>> checking would allow to skip reading the p2p device configuration file. >>> >>> After the netdev interface is created, the code creates an additional >>> interface for the P2P Device management (only if one does not exist >>> and the underlying device supports it). So in this case >>> wpas_p2p_add_p2pdev_interface() is called, and only in this case >>> iface->p2p_mgmt is set. >>> >>> So bottom line, I think that a cleaner fix would be to base the >>> reading on the setting of iface->p2p_mgmt. >> >> Related to this I created a patch that would make the -m option redundant. >> This is done by skipping networks and ignore 'p2p_disabled=1' >> in the configuration when iface->p2p_mgmt is set. >> > > Does this handle the case of update_config=1? In this case, if the configuration file is shared between 2 different interfaces, one of them will override the configuration of the other when wpa_supplicant exists or instructed to save the config. > > Regardless, I think that the approach of have one global configuration file to hold device wide configuration is cleaner and more scalable. Ah, I see. I thought the -m file contained P2P_DEVICE specific configuration, but you are saying it should contain configuration items applicable to all interfaces. I think the problem is that it is not straight-forward what the usage level is of the configuration items. Some are applicable to supplicant, some per-interface, and some only for a specific type of interface. Regards, Arend > Regards, > > Ilan.
> >> Related to this I created a patch that would make the -m option > redundant. > >> This is done by skipping networks and ignore 'p2p_disabled=1' > >> in the configuration when iface->p2p_mgmt is set. > >> > > > > Does this handle the case of update_config=1? In this case, if the > configuration file is shared between 2 different interfaces, one of them will > override the configuration of the other when wpa_supplicant exists or > instructed to save the config. > > > > Regardless, I think that the approach of have one global configuration file > to hold device wide configuration is cleaner and more scalable. > > Ah, I see. I thought the -m file contained P2P_DEVICE specific configuration, > but you are saying it should contain configuration items applicable to all > interfaces. I think the problem is that it is not straight-forward what the > usage level is of the configuration items. > Some are applicable to supplicant, some per-interface, and some only for a > specific type of interface. > The -m is not a good solution for handling global wide configuration but was introduced to overcome the clear issues we encountered having the P2P Device interface share the configuration file with the other interface. Jouni accepted the change but wants to have a solution with will better handle configuration items that are global. The below patches were an attempt to handle this: http://patchwork.ozlabs.org/patch/332033/ http://patchwork.ozlabs.org/patch/332034/ http://patchwork.ozlabs.org/patch/332035/ Regards, Ilan.
On 09/29/14 19:35, Peer, Ilan wrote: > >>>> Related to this I created a patch that would make the -m option >> redundant. >>>> This is done by skipping networks and ignore 'p2p_disabled=1' >>>> in the configuration when iface->p2p_mgmt is set. >>>> >>> >>> Does this handle the case of update_config=1? In this case, if the >> configuration file is shared between 2 different interfaces, one of them will >> override the configuration of the other when wpa_supplicant exists or >> instructed to save the config. >>> >>> Regardless, I think that the approach of have one global configuration file >> to hold device wide configuration is cleaner and more scalable. >> >> Ah, I see. I thought the -m file contained P2P_DEVICE specific configuration, >> but you are saying it should contain configuration items applicable to all >> interfaces. I think the problem is that it is not straight-forward what the >> usage level is of the configuration items. >> Some are applicable to supplicant, some per-interface, and some only for a >> specific type of interface. >> > > The -m is not a good solution for handling global wide configuration but was introduced to overcome the clear issues we encountered having the P2P Device interface share the configuration file with the other interface. Jouni accepted the change but wants to have a solution with will better handle configuration items that are global. The below patches were an attempt to handle this: I totally agree. When learning about the -m option I got a yuk feeling. I will check the patches. Thanks, Arend > http://patchwork.ozlabs.org/patch/332033/ > http://patchwork.ozlabs.org/patch/332034/ > http://patchwork.ozlabs.org/patch/332035/ > > Regards, > > Ilan.
diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index f20bc62..9d21fe0 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -3509,7 +3509,9 @@ static int wpa_supplicant_init_iface(struct wpa_supplicant *wpa_s, #ifdef CONFIG_P2P wpa_s->conf_p2p_dev = os_rel2abs_path(iface->conf_p2p_dev); +#ifndef ANDROID_P2P wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); +#endif #endif /* CONFIG_P2P */ /*
It will be loaded later for Non-netdev p2p interface. Change-Id: Ib53daf22c84f2581b499462dc09a74ccfbac226a Signed-off-by: Dmitry Shmidt <dimitrysh@google.com> --- wpa_supplicant/wpa_supplicant.c | 2 ++ 1 file changed, 2 insertions(+)