Hi Archie,
On Sun, Aug 21, 2022 at 9:54 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
>
> We set the pending settings flag when sending MGMT_SETTING_*
> commands to the MGMT layer and clear them when receiving success
> reply, but we don't clear them when receiving error reply. This
> might cause a setting to be stuck in pending state.
>
> Therefore, also clear the pending flag when receiving error.
> Furthermore, this patch also postpone setting the pending flag
> until we queue the MGMT command in order to avoid setting it too
> soon but we return early.
>
> Reviewed-by: Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx>
>
> ---
>
> src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index ec26aab1a7..4da1fcc3e5 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -640,14 +640,21 @@ static void new_settings_callback(uint16_t index, uint16_t length,
> settings_changed(adapter, settings);
> }
>
> +struct set_mode_data {
> + struct btd_adapter *adapter;
> + uint32_t setting;
> +};
> +
> static void set_mode_complete(uint8_t status, uint16_t length,
> const void *param, void *user_data)
> {
> - struct btd_adapter *adapter = user_data;
> + struct set_mode_data *data = user_data;
> + struct btd_adapter *adapter = data->adapter;
>
> if (status != MGMT_STATUS_SUCCESS) {
> btd_error(adapter->dev_id, "Failed to set mode: %s (0x%02x)",
> mgmt_errstr(status), status);
> + adapter->pending_settings &= ~data->setting;
> return;
> }
>
> @@ -677,6 +684,7 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> {
> struct mgmt_mode cp;
> uint32_t setting = 0;
> + struct set_mode_data *data;
>
> memset(&cp, 0, sizeof(cp));
> cp.val = mode;
> @@ -699,15 +707,23 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> break;
> }
>
> - adapter->pending_settings |= setting;
> -
> DBG("sending set mode command for index %u", adapter->dev_id);
>
> + data = g_try_new0(struct set_mode_data, 1);
Use new0 instead of g_try_new0.
> + if (!data)
> + goto failed;
> +
> + data->adapter = adapter;
> + data->setting = setting;
> +
> if (mgmt_send(adapter->mgmt, opcode,
> adapter->dev_id, sizeof(cp), &cp,
> - set_mode_complete, adapter, NULL) > 0)
> + set_mode_complete, data, g_free) > 0) {
> + adapter->pending_settings |= setting;
> return true;
> + }
>
> +failed:
> btd_error(adapter->dev_id, "Failed to set mode for index %u",
> adapter->dev_id);
>
> @@ -718,6 +734,7 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
> uint16_t timeout)
> {
> struct mgmt_cp_set_discoverable cp;
> + struct set_mode_data *data;
>
> memset(&cp, 0, sizeof(cp));
> cp.val = mode;
> @@ -734,11 +751,19 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
> mode);
> }
>
> + data = g_try_new0(struct set_mode_data, 1);
> + if (!data)
> + goto failed;
> +
> + data->adapter = adapter;
> + data->setting = 0;
> +
> if (mgmt_send(adapter->mgmt, MGMT_OP_SET_DISCOVERABLE,
> adapter->dev_id, sizeof(cp), &cp,
> - set_mode_complete, adapter, NULL) > 0)
> + set_mode_complete, data, g_free) > 0)
> return true;
>
> +failed:
> btd_error(adapter->dev_id, "Failed to set mode for index %u",
> adapter->dev_id);
Looks like the data pointer is leaked in case it fails to be sent/queued.
> @@ -2877,6 +2902,7 @@ static gboolean property_get_mode(struct btd_adapter *adapter,
>
> struct property_set_data {
> struct btd_adapter *adapter;
> + uint32_t setting;
> GDBusPendingPropertySet id;
> };
>
> @@ -2901,6 +2927,8 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
>
> g_dbus_pending_property_error(data->id, dbus_err,
> mgmt_errstr(status));
> +
> + adapter->pending_settings &= ~data->setting;
> return;
> }
>
> @@ -2969,8 +2997,6 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>
> mode = (enable == TRUE) ? 0x01 : 0x00;
>
> - adapter->pending_settings |= setting;
> -
> switch (setting) {
> case MGMT_SETTING_POWERED:
> opcode = MGMT_OP_SET_POWERED;
> @@ -3024,11 +3050,14 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
> goto failed;
>
> data->adapter = adapter;
> + data->setting = setting;
> data->id = id;
>
> if (mgmt_send(adapter->mgmt, opcode, adapter->dev_id, len, param,
> - property_set_mode_complete, data, g_free) > 0)
> + property_set_mode_complete, data, g_free) > 0) {
> + adapter->pending_settings |= setting;
> return;
> + }
>
> g_free(data);
>
> --
> 2.37.1.595.g718a3a8f04-goog
--
Luiz Augusto von Dentz
