Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CAEQQxWxKs7ewwVyq4mnsyLbRhErQe9vZc5joNK6zfGSO3wN5bg@mail.gmail.com>
 <d9eca20059088ad8bdaac70d7d98811166839b27.camel@hadess.net> <CAEQQxWyz-9iDU5Cs_NjHm81A6kdr0WUXPggp_2DTHeVVAQXUZQ@mail.gmail.com>
In-Reply-To: <CAEQQxWyz-9iDU5Cs_NjHm81A6kdr0WUXPggp_2DTHeVVAQXUZQ@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Wed, 8 Jul 2020 14:14:37 -0700
Message-ID: <CABBYNZLzQH5ow02xB61d8pF7pcxXoP7Boa06G-HNxJ0Q66FGRg@mail.gmail.com>
Subject: Re: Temporary device removal during discovery
To:     Andrey Batyiev <batyiev@gmail.com>
Cc:     Bastien Nocera <hadess@hadess.net>,
        linux-bluetooth <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Andrey,

On Wed, Jul 8, 2020 at 8:56 AM Andrey Batyiev <batyiev@gmail.com> wrote:
>
> Hi Bastien, everyone,
>
> I've made a patch to the bluez, so the temporary devices are routinely
> purged based on their last_seen attribute.
>
> What do you think about such solution?
>
> Thanks,
>     Andrey
>
>
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 529002b02..101b03633 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -88,6 +88,7 @@
>
> #define CONN_SCAN_TIMEOUT (3)
> #define IDLE_DISCOV_TIMEOUT (5)
> +#define TEMP_DEV_REAPER_INTERVAL (30)
> #define TEMP_DEV_TIMEOUT (3 * 60)
> #define BONDING_TIMEOUT (2 * 60)
>
> @@ -1491,18 +1492,18 @@ static gboolean remove_temp_devices(gpointer user_data)
>
>        DBG("%s", adapter->path);
>
> -       adapter->temp_devices_timeout = 0;
> -
>        for (l = adapter->devices; l != NULL; l = next) {
>                struct btd_device *dev = l->data;
>
>                next = g_slist_next(l);
>
> -               if (device_is_temporary(dev) && !btd_device_is_connected(dev))
> +               if (device_is_temporary(dev) &&
> +                   !btd_device_is_connected(dev) &&
> +                   device_last_seen_delta(dev) > TEMP_DEV_TIMEOUT)

The delta logic might be a nice addition as a separate patch, it is
more for detecting devices disappearing then actually cleanup during
power off.

>                        btd_adapter_remove_device(adapter, dev);
>        }
>
> -       return FALSE;
> +       return TRUE;
> }
>
> static void discovery_cleanup(struct btd_adapter *adapter)
> @@ -1516,11 +1517,6 @@ static void discovery_cleanup(struct
> btd_adapter *adapter)
>                adapter->discovery_idle_timeout = 0;
>        }
>
> -       if (adapter->temp_devices_timeout > 0) {
> -               g_source_remove(adapter->temp_devices_timeout);
> -               adapter->temp_devices_timeout = 0;
> -       }
> -
>        g_slist_free_full(adapter->discovery_found,
>                                                invalidate_rssi_and_tx_power);
>        adapter->discovery_found = NULL;
> @@ -1536,9 +1532,6 @@ static void discovery_cleanup(struct btd_adapter *adapter)
>                if (device_is_temporary(dev) && !device_is_connectable(dev))
>                        btd_adapter_remove_device(adapter, dev);
>        }
> -
> -       adapter->temp_devices_timeout = g_timeout_add_seconds(TEMP_DEV_TIMEOUT,
> -                                               remove_temp_devices, adapter);
> }
>
> static void discovery_free(void *user_data)
> @@ -2405,7 +2398,7 @@ static bool parse_pathloss(DBusMessageIter *value,
>        return true;
> }
>
> -static bool parse_transport(DBusMessageIter *value,
> +static bool parse_transport(DBusMessageIter *value,
>                                        struct discovery_filter *filter)
> {
>        char *transport_str;
> @@ -5362,6 +5355,11 @@ static void adapter_free(gpointer user_data)
>                adapter->passive_scan_timeout = 0;
>        }
>
> +       if (adapter->temp_devices_timeout > 0) {
> +               g_source_remove(adapter->temp_devices_timeout);
> +               adapter->temp_devices_timeout = 0;
> +       }
> +
>        if (adapter->load_ltks_timeout > 0)
>                g_source_remove(adapter->load_ltks_timeout);
>
> @@ -6343,6 +6341,9 @@ static struct btd_adapter *btd_adapter_new(uint16_t index)
>
>        adapter->auths = g_queue_new();
>
> +       adapter->temp_devices_timeout =
> g_timeout_add_seconds(TEMP_DEV_REAPER_INTERVAL,
> +                                               remove_temp_devices, adapter);
> +
>        return btd_adapter_ref(adapter);
> }
>
> diff --git a/src/device.c b/src/device.c
> index 0deee2707..cebbabab2 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4240,6 +4240,15 @@ void device_update_last_seen(struct btd_device
> *device, uint8_t bdaddr_type)
>                device->le_seen = time(NULL);
> }
>
> +time_t device_last_seen_delta(struct btd_device *device)
> +{
> +       const time_t now = time(NULL);
> +       const time_t bredr_time = now - device->bredr_seen;
> +       const time_t le_time = now - device->le_seen;
> +
> +       return bredr_time < le_time ? bredr_time : le_time;
> +}
> +
> /* It is possible that we have two device objects for the same device in
>  * case it has first been discovered over BR/EDR and has a private
>  * address when discovered over LE for the first time. In such a case we
> diff --git a/src/device.h b/src/device.h
> index cb8d884e8..75fd3ec60 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -44,6 +44,7 @@ void device_update_addr(struct btd_device *device,
> const bdaddr_t *bdaddr,
> void device_set_bredr_support(struct btd_device *device);
> void device_set_le_support(struct btd_device *device, uint8_t bdaddr_type);
> void device_update_last_seen(struct btd_device *device, uint8_t bdaddr_type);
> +time_t device_last_seen_delta(struct btd_device *device);
> void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup);
> uint32_t btd_device_get_class(struct btd_device *device);
> uint16_t btd_device_get_vendor(struct btd_device *device);


-- 
Luiz Augusto von Dentz