| Message ID | 20240117210557.2606474-1-kaidong@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix the wrapping of dBus container for SignalPoll | expand |
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 >
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..
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 --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;
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(-)