| Message ID | 20191011204925.107808-1-matthewmwang@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | dbus: move roam metrics | expand |
+cc Dan On Fri, Oct 11, 2019 at 1:49 PM Matthew Wang <matthewmwang@chromium.org> wrote: > > These properties were in the wpas_dbus_bss_properties array when > they should have been in the wpas_dbus_interface_properties array. Move > them to the right place. > > Signed-off-by: Matthew Wang <matthewmwang@chromium.org> > --- > wpa_supplicant/dbus/dbus_new.c | 48 +++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c > index 4df61f233..65ddcf657 100644 > --- a/wpa_supplicant/dbus/dbus_new.c > +++ b/wpa_supplicant/dbus/dbus_new.c > @@ -2855,30 +2855,6 @@ static const struct wpa_dbus_property_desc wpas_dbus_bss_properties[] = { > NULL, > NULL > }, > - { > - "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_roam_time, > - NULL, > - NULL > - }, > - { > - "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > - wpas_dbus_getter_roam_complete, > - NULL, > - NULL > - }, > - { > - "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_session_length, > - NULL, > - NULL > - }, > - { > - "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_bss_tm_status, > - NULL, > - NULL > - }, > { NULL, NULL, NULL, NULL, NULL, NULL } > }; > > @@ -3794,6 +3770,30 @@ static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = { > NULL, > NULL > }, > + { > + "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_roam_time, > + NULL, > + NULL > + }, > + { > + "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > + wpas_dbus_getter_roam_complete, > + NULL, > + NULL > + }, > + { > + "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_session_length, > + NULL, > + NULL > + }, > + { > + "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_bss_tm_status, > + NULL, > + NULL > + }, > #ifdef CONFIG_MESH > { "MeshPeers", WPAS_DBUS_NEW_IFACE_MESH, "aay", > wpas_dbus_getter_mesh_peers, > -- > 2.21.0 >
On Fri, 2019-10-11 at 13:49 -0700, Matthew Wang wrote: > These properties were in the wpas_dbus_bss_properties array when > they should have been in the wpas_dbus_interface_properties array. > Move > them to the right place. Haven't had time to check, but were these properties in a wpa_supplicant release already? Or just in git master so far? Dan > Signed-off-by: Matthew Wang <matthewmwang@chromium.org> > --- > wpa_supplicant/dbus/dbus_new.c | 48 +++++++++++++++++--------------- > -- > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/wpa_supplicant/dbus/dbus_new.c > b/wpa_supplicant/dbus/dbus_new.c > index 4df61f233..65ddcf657 100644 > --- a/wpa_supplicant/dbus/dbus_new.c > +++ b/wpa_supplicant/dbus/dbus_new.c > @@ -2855,30 +2855,6 @@ static const struct wpa_dbus_property_desc > wpas_dbus_bss_properties[] = { > NULL, > NULL > }, > - { > - "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_roam_time, > - NULL, > - NULL > - }, > - { > - "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > - wpas_dbus_getter_roam_complete, > - NULL, > - NULL > - }, > - { > - "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_session_length, > - NULL, > - NULL > - }, > - { > - "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > - wpas_dbus_getter_bss_tm_status, > - NULL, > - NULL > - }, > { NULL, NULL, NULL, NULL, NULL, NULL } > }; > > @@ -3794,6 +3770,30 @@ static const struct wpa_dbus_property_desc > wpas_dbus_interface_properties[] = { > NULL, > NULL > }, > + { > + "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_roam_time, > + NULL, > + NULL > + }, > + { > + "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > + wpas_dbus_getter_roam_complete, > + NULL, > + NULL > + }, > + { > + "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_session_length, > + NULL, > + NULL > + }, > + { > + "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > + wpas_dbus_getter_bss_tm_status, > + NULL, > + NULL > + }, > #ifdef CONFIG_MESH > { "MeshPeers", WPAS_DBUS_NEW_IFACE_MESH, "aay", > wpas_dbus_getter_mesh_peers,
They are in a release. The original patch is here: https://w1.fi/cgit/hostap/commit/?id=2bbad1c7c9cbedf16eea122c9376e65691213108 On Thu, Oct 17, 2019 at 6:35 PM Dan Williams <dcbw@redhat.com> wrote: > > On Fri, 2019-10-11 at 13:49 -0700, Matthew Wang wrote: > > These properties were in the wpas_dbus_bss_properties array when > > they should have been in the wpas_dbus_interface_properties array. > > Move > > them to the right place. > > Haven't had time to check, but were these properties in a > wpa_supplicant release already? Or just in git master so far? > > Dan > > > Signed-off-by: Matthew Wang <matthewmwang@chromium.org> > > --- > > wpa_supplicant/dbus/dbus_new.c | 48 +++++++++++++++++--------------- > > -- > > 1 file changed, 24 insertions(+), 24 deletions(-) > > > > diff --git a/wpa_supplicant/dbus/dbus_new.c > > b/wpa_supplicant/dbus/dbus_new.c > > index 4df61f233..65ddcf657 100644 > > --- a/wpa_supplicant/dbus/dbus_new.c > > +++ b/wpa_supplicant/dbus/dbus_new.c > > @@ -2855,30 +2855,6 @@ static const struct wpa_dbus_property_desc > > wpas_dbus_bss_properties[] = { > > NULL, > > NULL > > }, > > - { > > - "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > - wpas_dbus_getter_roam_time, > > - NULL, > > - NULL > > - }, > > - { > > - "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > > - wpas_dbus_getter_roam_complete, > > - NULL, > > - NULL > > - }, > > - { > > - "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > - wpas_dbus_getter_session_length, > > - NULL, > > - NULL > > - }, > > - { > > - "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > - wpas_dbus_getter_bss_tm_status, > > - NULL, > > - NULL > > - }, > > { NULL, NULL, NULL, NULL, NULL, NULL } > > }; > > > > @@ -3794,6 +3770,30 @@ static const struct wpa_dbus_property_desc > > wpas_dbus_interface_properties[] = { > > NULL, > > NULL > > }, > > + { > > + "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > + wpas_dbus_getter_roam_time, > > + NULL, > > + NULL > > + }, > > + { > > + "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", > > + wpas_dbus_getter_roam_complete, > > + NULL, > > + NULL > > + }, > > + { > > + "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > + wpas_dbus_getter_session_length, > > + NULL, > > + NULL > > + }, > > + { > > + "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", > > + wpas_dbus_getter_bss_tm_status, > > + NULL, > > + NULL > > + }, > > #ifdef CONFIG_MESH > > { "MeshPeers", WPAS_DBUS_NEW_IFACE_MESH, "aay", > > wpas_dbus_getter_mesh_peers, >
On Thu, Oct 17, 2019 at 6:42 PM Matthew Wang <matthewmwang@google.com> wrote: (rearranged for non-top-post) > On Thu, Oct 17, 2019 at 6:35 PM Dan Williams <dcbw@redhat.com> wrote: > > > > On Fri, 2019-10-11 at 13:49 -0700, Matthew Wang wrote: > > > These properties were in the wpas_dbus_bss_properties array when > > > they should have been in the wpas_dbus_interface_properties array. > > > Move > > > them to the right place. > > > > Haven't had time to check, but were these properties in a > > wpa_supplicant release already? Or just in git master so far? > > They are in a release. The original patch is here: > https://w1.fi/cgit/hostap/commit/?id=2bbad1c7c9cbedf16eea122c9376e65691213108 Ping? This clearly isn't what was intended, and AFAICT, the current behavior is totally buggy -- it will take interpret the 'user_data' pointer incorrectly and produce garbage instead. So can't we just take this as a bugfix without quibbling about the potential ABI break? I would bet money that the only user for these so far is Chrome OS, where our downstream variant didn't have this bug (well, not until we pulled this mis-patched version in...). Brian
On Fri, Oct 11, 2019 at 01:49:25PM -0700, Matthew Wang wrote: > These properties were in the wpas_dbus_bss_properties array when > they should have been in the wpas_dbus_interface_properties array. Move > them to the right place. Thanks, applied with a bit more detailed commit message explaining why this is a fix to meet the original design and documented functionality rather than an actual change to the interface.
diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c index 4df61f233..65ddcf657 100644 --- a/wpa_supplicant/dbus/dbus_new.c +++ b/wpa_supplicant/dbus/dbus_new.c @@ -2855,30 +2855,6 @@ static const struct wpa_dbus_property_desc wpas_dbus_bss_properties[] = { NULL, NULL }, - { - "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", - wpas_dbus_getter_roam_time, - NULL, - NULL - }, - { - "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", - wpas_dbus_getter_roam_complete, - NULL, - NULL - }, - { - "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", - wpas_dbus_getter_session_length, - NULL, - NULL - }, - { - "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", - wpas_dbus_getter_bss_tm_status, - NULL, - NULL - }, { NULL, NULL, NULL, NULL, NULL, NULL } }; @@ -3794,6 +3770,30 @@ static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = { NULL, NULL }, + { + "RoamTime", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", + wpas_dbus_getter_roam_time, + NULL, + NULL + }, + { + "RoamComplete", WPAS_DBUS_NEW_IFACE_INTERFACE, "b", + wpas_dbus_getter_roam_complete, + NULL, + NULL + }, + { + "SessionLength", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", + wpas_dbus_getter_session_length, + NULL, + NULL + }, + { + "BSSTMStatus", WPAS_DBUS_NEW_IFACE_INTERFACE, "u", + wpas_dbus_getter_bss_tm_status, + NULL, + NULL + }, #ifdef CONFIG_MESH { "MeshPeers", WPAS_DBUS_NEW_IFACE_MESH, "aay", wpas_dbus_getter_mesh_peers,
These properties were in the wpas_dbus_bss_properties array when they should have been in the wpas_dbus_interface_properties array. Move them to the right place. Signed-off-by: Matthew Wang <matthewmwang@chromium.org> --- wpa_supplicant/dbus/dbus_new.c | 48 +++++++++++++++++----------------- 1 file changed, 24 insertions(+), 24 deletions(-)