| Message ID | 20141003150344.GA7252@w1.fi |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Oct 3, 2014 at 8:03 AM, Jouni Malinen <j@w1.fi> wrote: > On Wed, Sep 24, 2014 at 10:38:31AM -0700, Dmitry Shmidt wrote: >> It will be loaded later for Non-netdev p2p interface. > >> diff --git 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 */ > > I'm not sure I've ever fully understood how this -m argument would be > used. Anyway, this does not look correct. While going through the > implementation, I could not understand why conf_p2p_dev is added on top > of the per-interface configuration (wpa_s->confname) rather than using > conf_p2p_dev to replace wpa_s->confname. I've never used this -m > argument myself, so don't know what may or may not work if this is done. > Anyway, the following changes would look much simpler way of doing this > (and the still pending item of different configuration method would > likely be even cleaner). > > > diff --git a/wpa_supplicant/main.c b/wpa_supplicant/main.c > index d2e839d..e596468 100644 > --- a/wpa_supplicant/main.c > +++ b/wpa_supplicant/main.c > @@ -331,7 +331,8 @@ int main(int argc, char *argv[]) > if (wpa_s->global->p2p == NULL && > (wpa_s->drv_flags & > WPA_DRIVER_FLAGS_DEDICATED_P2P_DEVICE) && > - wpas_p2p_add_p2pdev_interface(wpa_s) < 0) > + wpas_p2p_add_p2pdev_interface(wpa_s, iface->conf_p2p_dev) < > + 0) > exitcode = -1; > #endif /* CONFIG_P2P */ > } > diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c > index 1189a6d..70a6382 100644 > --- a/wpa_supplicant/p2p_supplicant.c > +++ b/wpa_supplicant/p2p_supplicant.c > @@ -3786,7 +3786,8 @@ static void wpas_p2p_debug_print(void *ctx, int level, const char *msg) > } > > > -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s) > +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, > + const char *conf_p2p_dev) > { > struct wpa_interface iface; > struct wpa_supplicant *p2pdev_wpa_s; > @@ -3817,8 +3818,8 @@ int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s) > * If a P2P Device configuration file was given, use it as the interface > * configuration file (instead of using parent's configuration file. > */ > - if (wpa_s->conf_p2p_dev) { > - iface.confname = wpa_s->conf_p2p_dev; > + if (conf_p2p_dev) { > + iface.confname = conf_p2p_dev; > iface.ctrl_interface = NULL; > } else { > iface.confname = wpa_s->confname; > diff --git a/wpa_supplicant/p2p_supplicant.h b/wpa_supplicant/p2p_supplicant.h > index 841d6df..b61d57f 100644 > --- a/wpa_supplicant/p2p_supplicant.h > +++ b/wpa_supplicant/p2p_supplicant.h > @@ -16,7 +16,8 @@ struct p2p_peer_info; > struct p2p_channels; > struct wps_event_fail; > > -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s); > +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, > + const char *conf_p2p_dev); > struct wpa_supplicant * wpas_get_p2p_go_iface(struct wpa_supplicant *wpa_s, > const u8 *ssid, size_t ssid_len); > struct wpa_supplicant * wpas_get_p2p_client_iface(struct wpa_supplicant *wpa_s, > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > index 3f60d2b..30d5540 100644 > --- a/wpa_supplicant/wpa_supplicant.c > +++ b/wpa_supplicant/wpa_supplicant.c > @@ -403,11 +403,6 @@ static void wpa_supplicant_cleanup(struct wpa_supplicant *wpa_s) > os_free(wpa_s->confanother); > wpa_s->confanother = NULL; > > -#ifdef CONFIG_P2P > - os_free(wpa_s->conf_p2p_dev); > - wpa_s->conf_p2p_dev = NULL; > -#endif /* CONFIG_P2P */ > - > wpa_sm_set_eapol(wpa_s->wpa, NULL); > eapol_sm_deinit(wpa_s->eapol); > wpa_s->eapol = NULL; > @@ -3653,11 +3648,6 @@ static int wpa_supplicant_init_iface(struct wpa_supplicant *wpa_s, > wpa_s->confanother = os_rel2abs_path(iface->confanother); > wpa_config_read(wpa_s->confanother, wpa_s->conf); > > -#ifdef CONFIG_P2P > - wpa_s->conf_p2p_dev = os_rel2abs_path(iface->conf_p2p_dev); > - wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > -#endif /* CONFIG_P2P */ > - > /* > * Override ctrl_interface and driver_param if set on command > * line. > diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h > index f941923..f375486 100644 > --- a/wpa_supplicant/wpa_supplicant_i.h > +++ b/wpa_supplicant/wpa_supplicant_i.h > @@ -67,7 +67,7 @@ struct wpa_interface { > > #ifdef CONFIG_P2P > /** > - * conf_p2p_dev - Additional configuration file used to hold the > + * conf_p2p_dev - Configuration file used to hold the > * P2P Device configuration parameters. > * > * This can also be %NULL. In such a case, if a P2P Device dedicated > @@ -408,10 +408,6 @@ struct wpa_supplicant { > char *confname; > char *confanother; > > -#ifdef CONFIG_P2P > - char *conf_p2p_dev; > -#endif /* CONFIG_P2P */ > - > struct wpa_config *conf; > int countermeasures; > struct os_reltime last_michael_mic_error; > > -- > Jouni Malinen PGP id EFC895FA This patch works for me, thanks!
On Fri, Oct 03, 2014 at 10:50:42AM -0700, Dmitry Shmidt wrote: > On Fri, Oct 3, 2014 at 8:03 AM, Jouni Malinen <j@w1.fi> wrote: > > I'm not sure I've ever fully understood how this -m argument would be > > used. Anyway, this does not look correct. While going through the > > implementation, I could not understand why conf_p2p_dev is added on top > > of the per-interface configuration (wpa_s->confname) rather than using > > conf_p2p_dev to replace wpa_s->confname. I've never used this -m > > argument myself, so don't know what may or may not work if this is done. > > Anyway, the following changes would look much simpler way of doing this > > (and the still pending item of different configuration method would > > likely be even cleaner). ... > This patch works for me, thanks! Thanks. I applied this now. If there are other users of -m that have issues with the change, please let me know and lets figure out a more suitable solution to fit all existing users.
> -----Original Message----- > From: hostap-bounces@lists.shmoo.com [mailto:hostap- > bounces@lists.shmoo.com] On Behalf Of Jouni Malinen > Sent: Friday, October 03, 2014 18:04 > To: Dmitry Shmidt > Cc: hostap@lists.shmoo.com > Subject: Re: [PATCH] Android: Skip explicit conf_p2p_dev loading for main > interface > > On Wed, Sep 24, 2014 at 10:38:31AM -0700, Dmitry Shmidt wrote: > > It will be loaded later for Non-netdev p2p interface. > > > diff --git 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 */ > > I'm not sure I've ever fully understood how this -m argument would be used. > Anyway, this does not look correct. While going through the implementation, The intention was to have a separate configuration file for the P2P Device interface, as it cannot simply (and I do not think it should) share the configuration file of that of the netdev interface (for example using update_config=1 would break things). As you state in the past, this is not the solution that you want to have at the end, but rather prefer to have some global configuration file, that would be shared between all the interfaces. > I could not understand why conf_p2p_dev is added on top of the per- > interface configuration (wpa_s->confname) rather than using conf_p2p_dev > to replace wpa_s->confname. I've never used this -m argument myself, so > don't know what may or may not work if this is done. > Anyway, the following changes would look much simpler way of doing this > (and the still pending item of different configuration method would likely be > even cleaner). > > > diff --git a/wpa_supplicant/main.c b/wpa_supplicant/main.c index > d2e839d..e596468 100644 > --- a/wpa_supplicant/main.c > +++ b/wpa_supplicant/main.c > @@ -331,7 +331,8 @@ int main(int argc, char *argv[]) > if (wpa_s->global->p2p == NULL && > (wpa_s->drv_flags & > WPA_DRIVER_FLAGS_DEDICATED_P2P_DEVICE) && > - wpas_p2p_add_p2pdev_interface(wpa_s) < 0) > + wpas_p2p_add_p2pdev_interface(wpa_s, iface- > >conf_p2p_dev) < > + 0) > exitcode = -1; > #endif /* CONFIG_P2P */ > } > diff --git a/wpa_supplicant/p2p_supplicant.c > b/wpa_supplicant/p2p_supplicant.c index 1189a6d..70a6382 100644 > --- a/wpa_supplicant/p2p_supplicant.c > +++ b/wpa_supplicant/p2p_supplicant.c > @@ -3786,7 +3786,8 @@ static void wpas_p2p_debug_print(void *ctx, int > level, const char *msg) } > > > -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s) > +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, > + const char *conf_p2p_dev) > { > struct wpa_interface iface; > struct wpa_supplicant *p2pdev_wpa_s; > @@ -3817,8 +3818,8 @@ int wpas_p2p_add_p2pdev_interface(struct > wpa_supplicant *wpa_s) > * If a P2P Device configuration file was given, use it as the interface > * configuration file (instead of using parent's configuration file. > */ > - if (wpa_s->conf_p2p_dev) { > - iface.confname = wpa_s->conf_p2p_dev; > + if (conf_p2p_dev) { > + iface.confname = conf_p2p_dev; > iface.ctrl_interface = NULL; > } else { > iface.confname = wpa_s->confname; > diff --git a/wpa_supplicant/p2p_supplicant.h > b/wpa_supplicant/p2p_supplicant.h index 841d6df..b61d57f 100644 > --- a/wpa_supplicant/p2p_supplicant.h > +++ b/wpa_supplicant/p2p_supplicant.h > @@ -16,7 +16,8 @@ struct p2p_peer_info; > struct p2p_channels; > struct wps_event_fail; > > -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s); > +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, > + const char *conf_p2p_dev); > struct wpa_supplicant * wpas_get_p2p_go_iface(struct wpa_supplicant > *wpa_s, > const u8 *ssid, size_t ssid_len); > struct wpa_supplicant * wpas_get_p2p_client_iface(struct wpa_supplicant > *wpa_s, diff --git a/wpa_supplicant/wpa_supplicant.c > b/wpa_supplicant/wpa_supplicant.c index 3f60d2b..30d5540 100644 > --- a/wpa_supplicant/wpa_supplicant.c > +++ b/wpa_supplicant/wpa_supplicant.c > @@ -403,11 +403,6 @@ static void wpa_supplicant_cleanup(struct > wpa_supplicant *wpa_s) > os_free(wpa_s->confanother); > wpa_s->confanother = NULL; > > -#ifdef CONFIG_P2P > - os_free(wpa_s->conf_p2p_dev); > - wpa_s->conf_p2p_dev = NULL; > -#endif /* CONFIG_P2P */ > - > wpa_sm_set_eapol(wpa_s->wpa, NULL); > eapol_sm_deinit(wpa_s->eapol); > wpa_s->eapol = NULL; > @@ -3653,11 +3648,6 @@ static int wpa_supplicant_init_iface(struct > wpa_supplicant *wpa_s, > wpa_s->confanother = os_rel2abs_path(iface- > >confanother); > wpa_config_read(wpa_s->confanother, wpa_s->conf); > > -#ifdef CONFIG_P2P > - wpa_s->conf_p2p_dev = os_rel2abs_path(iface- > >conf_p2p_dev); > - wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); > -#endif /* CONFIG_P2P */ > - > /* > * Override ctrl_interface and driver_param if set on > command > * line. > diff --git a/wpa_supplicant/wpa_supplicant_i.h > b/wpa_supplicant/wpa_supplicant_i.h > index f941923..f375486 100644 > --- a/wpa_supplicant/wpa_supplicant_i.h > +++ b/wpa_supplicant/wpa_supplicant_i.h > @@ -67,7 +67,7 @@ struct wpa_interface { > > #ifdef CONFIG_P2P > /** > - * conf_p2p_dev - Additional configuration file used to hold the > + * conf_p2p_dev - Configuration file used to hold the > * P2P Device configuration parameters. > * > * This can also be %NULL. In such a case, if a P2P Device dedicated > @@ -408,10 +408,6 @@ struct wpa_supplicant { > char *confname; > char *confanother; > > -#ifdef CONFIG_P2P > - char *conf_p2p_dev; > -#endif /* CONFIG_P2P */ > - > struct wpa_config *conf; > int countermeasures; > struct os_reltime last_michael_mic_error; > Tested this and it looks ok. Regards, Ilan.
diff --git a/wpa_supplicant/main.c b/wpa_supplicant/main.c index d2e839d..e596468 100644 --- a/wpa_supplicant/main.c +++ b/wpa_supplicant/main.c @@ -331,7 +331,8 @@ int main(int argc, char *argv[]) if (wpa_s->global->p2p == NULL && (wpa_s->drv_flags & WPA_DRIVER_FLAGS_DEDICATED_P2P_DEVICE) && - wpas_p2p_add_p2pdev_interface(wpa_s) < 0) + wpas_p2p_add_p2pdev_interface(wpa_s, iface->conf_p2p_dev) < + 0) exitcode = -1; #endif /* CONFIG_P2P */ } diff --git a/wpa_supplicant/p2p_supplicant.c b/wpa_supplicant/p2p_supplicant.c index 1189a6d..70a6382 100644 --- a/wpa_supplicant/p2p_supplicant.c +++ b/wpa_supplicant/p2p_supplicant.c @@ -3786,7 +3786,8 @@ static void wpas_p2p_debug_print(void *ctx, int level, const char *msg) } -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s) +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, + const char *conf_p2p_dev) { struct wpa_interface iface; struct wpa_supplicant *p2pdev_wpa_s; @@ -3817,8 +3818,8 @@ int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s) * If a P2P Device configuration file was given, use it as the interface * configuration file (instead of using parent's configuration file. */ - if (wpa_s->conf_p2p_dev) { - iface.confname = wpa_s->conf_p2p_dev; + if (conf_p2p_dev) { + iface.confname = conf_p2p_dev; iface.ctrl_interface = NULL; } else { iface.confname = wpa_s->confname; diff --git a/wpa_supplicant/p2p_supplicant.h b/wpa_supplicant/p2p_supplicant.h index 841d6df..b61d57f 100644 --- a/wpa_supplicant/p2p_supplicant.h +++ b/wpa_supplicant/p2p_supplicant.h @@ -16,7 +16,8 @@ struct p2p_peer_info; struct p2p_channels; struct wps_event_fail; -int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s); +int wpas_p2p_add_p2pdev_interface(struct wpa_supplicant *wpa_s, + const char *conf_p2p_dev); struct wpa_supplicant * wpas_get_p2p_go_iface(struct wpa_supplicant *wpa_s, const u8 *ssid, size_t ssid_len); struct wpa_supplicant * wpas_get_p2p_client_iface(struct wpa_supplicant *wpa_s, diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c index 3f60d2b..30d5540 100644 --- a/wpa_supplicant/wpa_supplicant.c +++ b/wpa_supplicant/wpa_supplicant.c @@ -403,11 +403,6 @@ static void wpa_supplicant_cleanup(struct wpa_supplicant *wpa_s) os_free(wpa_s->confanother); wpa_s->confanother = NULL; -#ifdef CONFIG_P2P - os_free(wpa_s->conf_p2p_dev); - wpa_s->conf_p2p_dev = NULL; -#endif /* CONFIG_P2P */ - wpa_sm_set_eapol(wpa_s->wpa, NULL); eapol_sm_deinit(wpa_s->eapol); wpa_s->eapol = NULL; @@ -3653,11 +3648,6 @@ static int wpa_supplicant_init_iface(struct wpa_supplicant *wpa_s, wpa_s->confanother = os_rel2abs_path(iface->confanother); wpa_config_read(wpa_s->confanother, wpa_s->conf); -#ifdef CONFIG_P2P - wpa_s->conf_p2p_dev = os_rel2abs_path(iface->conf_p2p_dev); - wpa_config_read(wpa_s->conf_p2p_dev, wpa_s->conf); -#endif /* CONFIG_P2P */ - /* * Override ctrl_interface and driver_param if set on command * line. diff --git a/wpa_supplicant/wpa_supplicant_i.h b/wpa_supplicant/wpa_supplicant_i.h index f941923..f375486 100644 --- a/wpa_supplicant/wpa_supplicant_i.h +++ b/wpa_supplicant/wpa_supplicant_i.h @@ -67,7 +67,7 @@ struct wpa_interface { #ifdef CONFIG_P2P /** - * conf_p2p_dev - Additional configuration file used to hold the + * conf_p2p_dev - Configuration file used to hold the * P2P Device configuration parameters. * * This can also be %NULL. In such a case, if a P2P Device dedicated @@ -408,10 +408,6 @@ struct wpa_supplicant { char *confname; char *confanother; -#ifdef CONFIG_P2P - char *conf_p2p_dev; -#endif /* CONFIG_P2P */ - struct wpa_config *conf; int countermeasures; struct os_reltime last_michael_mic_error;