diff mbox series

Fix the wrapping of dBus container for SignalPoll

Message ID 20240117210557.2606474-1-kaidong@chromium.org
State New
Headers show
Series Fix the wrapping of dBus container for SignalPoll | expand

Commit Message

Kaidong Wang Jan. 17, 2024, 9:05 p.m. UTC
SignalPoll is expected to return a dict, while the code wraps the dBus
container as a variant, which leads to implicit unwrapping of variant.

Wrap the dBus container as a dict for SignalPoll.

Signed-off-by: Kaidong Wang <kaidong@chromium.org>
---
 wpa_supplicant/dbus/dbus_new_helpers.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Kaidong Wang Jan. 18, 2024, 7:16 p.m. UTC | #1
This patch removes the wrapping of variants for SignalPoll, but I saw
this wrapping also used for all dict containers. Is this wrapping
expected?

Best regards
Kaidong

Best regards
Kaidong


On Wed, Jan 17, 2024 at 1:06 PM Kaidong Wang <kaidong@chromium.org> wrote:
>
> SignalPoll is expected to return a dict, while the code wraps the dBus
> container as a variant, which leads to implicit unwrapping of variant.
>
> Wrap the dBus container as a dict for SignalPoll.
>
> Signed-off-by: Kaidong Wang <kaidong@chromium.org>
> ---
>  wpa_supplicant/dbus/dbus_new_helpers.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/wpa_supplicant/dbus/dbus_new_helpers.c b/wpa_supplicant/dbus/dbus_new_helpers.c
> index 7fb066991..bac66b87b 100644
> --- a/wpa_supplicant/dbus/dbus_new_helpers.c
> +++ b/wpa_supplicant/dbus/dbus_new_helpers.c
> @@ -1046,7 +1046,7 @@ static double guard_interval_to_double(enum guard_interval value)
>  /**
>   * wpas_dbus_new_from_signal_information - Adds a wpa_signal_info
>   * to a DBusMessage.
> - * @msg: Pointer to message to append fields to
> + * @iter: Pointer to message to append fields to
>   * @si: Pointer to wpa_signal_info to add to the message
>   * Returns: 0 on success, otherwise, an errorcode
>   *
> @@ -1057,11 +1057,9 @@ static double guard_interval_to_double(enum guard_interval value)
>  int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
>                                           struct wpa_signal_info *si)
>  {
> -       DBusMessageIter iter_dict, variant_iter;
> +       DBusMessageIter iter_dict;
>
> -       if (!dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
> -                                             "a{sv}", &variant_iter) ||
> -           !wpa_dbus_dict_open_write(&variant_iter, &iter_dict) ||
> +       if (!wpa_dbus_dict_open_write(iter, &iter_dict) ||
>             !wpa_dbus_dict_append_int32(&iter_dict, "rssi",
>                                         si->data.signal) ||
>             !wpa_dbus_dict_append_uint32(&iter_dict, "linkspeed",
> @@ -1180,8 +1178,7 @@ int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
>             ((si->data.flags & STA_DRV_DATA_TX_HE_DCM) &&
>              !wpa_dbus_dict_append_bool(&iter_dict, "tx-dcm",
>                                         si->data.tx_dcm)) ||
> -           !wpa_dbus_dict_close_write(&variant_iter, &iter_dict) ||
> -           !dbus_message_iter_close_container(iter, &variant_iter))
> +               !wpa_dbus_dict_close_write(iter, &iter_dict))
>                 return -ENOMEM;
>
>         return 0;
> --
> 2.43.0.381.gb435a96ce8-goog
>
Jouni Malinen Jan. 20, 2024, 6:13 p.m. UTC | #2
On Thu, Jan 18, 2024 at 11:16:06AM -0800, Kaidong Wang wrote:
> This patch removes the wrapping of variants for SignalPoll, but I saw
> this wrapping also used for all dict containers. Is this wrapping
> expected?

I have to admit that I do not know why that design was used, but it was
present from the very first commit that added the new DBus API in 2009..
Kaidong Wang Jan. 22, 2024, 10:38 p.m. UTC | #3
Thanks for the information!
https://w1.fi/wpa_supplicant/devel/dbus.html shows SignalPoll returns
a{sv} properties, while the code wraps the dict as a variant, which
causes implicit unwrapping in our code.
Similar wrapping is also seen in other places such as
wpas_dbus_simple_array_property_getter(), wpas_dbus_getter_bss_wps(),
wpas_dbus_getter_network_properties(). If we want to maintain this
behavior for compatibility or whatever reason, we might update the
documentation to clarify that it is WAI, otherwise I can submit
another patch to remove all these variant wrappings.

Best regards
Kaidong

On Sat, Jan 20, 2024 at 10:13 AM Jouni Malinen <j@w1.fi> wrote:
>
> On Thu, Jan 18, 2024 at 11:16:06AM -0800, Kaidong Wang wrote:
> > This patch removes the wrapping of variants for SignalPoll, but I saw
> > this wrapping also used for all dict containers. Is this wrapping
> > expected?
>
> I have to admit that I do not know why that design was used, but it was
> present from the very first commit that added the new DBus API in 2009..
>
> --
> Jouni Malinen                                            PGP id EFC895FA
diff mbox series

Patch

diff --git a/wpa_supplicant/dbus/dbus_new_helpers.c b/wpa_supplicant/dbus/dbus_new_helpers.c
index 7fb066991..bac66b87b 100644
--- a/wpa_supplicant/dbus/dbus_new_helpers.c
+++ b/wpa_supplicant/dbus/dbus_new_helpers.c
@@ -1046,7 +1046,7 @@  static double guard_interval_to_double(enum guard_interval value)
 /**
  * wpas_dbus_new_from_signal_information - Adds a wpa_signal_info
  * to a DBusMessage.
- * @msg: Pointer to message to append fields to
+ * @iter: Pointer to message to append fields to
  * @si: Pointer to wpa_signal_info to add to the message
  * Returns: 0 on success, otherwise, an errorcode
  *
@@ -1057,11 +1057,9 @@  static double guard_interval_to_double(enum guard_interval value)
 int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
 					  struct wpa_signal_info *si)
 {
-	DBusMessageIter iter_dict, variant_iter;
+	DBusMessageIter iter_dict;
 
-	if (!dbus_message_iter_open_container(iter, DBUS_TYPE_VARIANT,
-					      "a{sv}", &variant_iter) ||
-	    !wpa_dbus_dict_open_write(&variant_iter, &iter_dict) ||
+	if (!wpa_dbus_dict_open_write(iter, &iter_dict) ||
 	    !wpa_dbus_dict_append_int32(&iter_dict, "rssi",
 					si->data.signal) ||
 	    !wpa_dbus_dict_append_uint32(&iter_dict, "linkspeed",
@@ -1180,8 +1178,7 @@  int wpas_dbus_new_from_signal_information(DBusMessageIter *iter,
 	    ((si->data.flags & STA_DRV_DATA_TX_HE_DCM) &&
 	     !wpa_dbus_dict_append_bool(&iter_dict, "tx-dcm",
 					si->data.tx_dcm)) ||
-	    !wpa_dbus_dict_close_write(&variant_iter, &iter_dict) ||
-	    !dbus_message_iter_close_container(iter, &variant_iter))
+		!wpa_dbus_dict_close_write(iter, &iter_dict))
 		return -ENOMEM;
 
 	return 0;