* RE: [Bluez] gatt: Use the same crypto instance for each device
2021-01-15 12:25 [Bluez PATCH] gatt: Use the same crypto instance for each device Archie Pusaka
@ 2021-01-15 13:37 ` bluez.test.bot
2021-01-15 18:39 ` [Bluez PATCH] " Luiz Augusto von Dentz
1 sibling, 0 replies; 3+ messages in thread
From: bluez.test.bot @ 2021-01-15 13:37 UTC (permalink / raw)
To: linux-bluetooth, apusaka
[-- Attachment #1: Type: text/plain, Size: 9515 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=415407
---Test result---
##############################
Test: CheckPatch - PASS
##############################
Test: CheckGitLint - PASS
##############################
Test: CheckBuild - FAIL
Output:
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:17:19: error: redundant redeclaration of ‘bt_crypto_new’ [-Werror=redundant-decls]
17 | struct bt_crypto *bt_crypto_new(void);
| ^~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:17:19: note: previous declaration of ‘bt_crypto_new’ was here
17 | struct bt_crypto *bt_crypto_new(void);
| ^~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:19:19: error: redundant redeclaration of ‘bt_crypto_ref’ [-Werror=redundant-decls]
19 | struct bt_crypto *bt_crypto_ref(struct bt_crypto *crypto);
| ^~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:19:19: note: previous declaration of ‘bt_crypto_ref’ was here
19 | struct bt_crypto *bt_crypto_ref(struct bt_crypto *crypto);
| ^~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:20:6: error: redundant redeclaration of ‘bt_crypto_unref’ [-Werror=redundant-decls]
20 | void bt_crypto_unref(struct bt_crypto *crypto);
| ^~~~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:20:6: note: previous declaration of ‘bt_crypto_unref’ was here
20 | void bt_crypto_unref(struct bt_crypto *crypto);
| ^~~~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:22:6: error: redundant redeclaration of ‘bt_crypto_random_bytes’ [-Werror=redundant-decls]
22 | bool bt_crypto_random_bytes(struct bt_crypto *crypto,
| ^~~~~~~~~~~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:22:6: note: previous declaration of ‘bt_crypto_random_bytes’ was here
22 | bool bt_crypto_random_bytes(struct bt_crypto *crypto,
| ^~~~~~~~~~~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:25:6: error: redundant redeclaration of ‘bt_crypto_e’ [-Werror=redundant-decls]
25 | bool bt_crypto_e(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:25:6: note: previous declaration of ‘bt_crypto_e’ was here
25 | bool bt_crypto_e(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:27:6: error: redundant redeclaration of ‘bt_crypto_ah’ [-Werror=redundant-decls]
27 | bool bt_crypto_ah(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:27:6: note: previous declaration of ‘bt_crypto_ah’ was here
27 | bool bt_crypto_ah(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:29:6: error: redundant redeclaration of ‘bt_crypto_c1’ [-Werror=redundant-decls]
29 | bool bt_crypto_c1(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:29:6: note: previous declaration of ‘bt_crypto_c1’ was here
29 | bool bt_crypto_c1(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:34:6: error: redundant redeclaration of ‘bt_crypto_s1’ [-Werror=redundant-decls]
34 | bool bt_crypto_s1(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:34:6: note: previous declaration of ‘bt_crypto_s1’ was here
34 | bool bt_crypto_s1(struct bt_crypto *crypto, const uint8_t k[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:37:6: error: redundant redeclaration of ‘bt_crypto_f4’ [-Werror=redundant-decls]
37 | bool bt_crypto_f4(struct bt_crypto *crypto, uint8_t u[32], uint8_t v[32],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:37:6: note: previous declaration of ‘bt_crypto_f4’ was here
37 | bool bt_crypto_f4(struct bt_crypto *crypto, uint8_t u[32], uint8_t v[32],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:39:6: error: redundant redeclaration of ‘bt_crypto_f5’ [-Werror=redundant-decls]
39 | bool bt_crypto_f5(struct bt_crypto *crypto, uint8_t w[32], uint8_t n1[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:39:6: note: previous declaration of ‘bt_crypto_f5’ was here
39 | bool bt_crypto_f5(struct bt_crypto *crypto, uint8_t w[32], uint8_t n1[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:42:6: error: redundant redeclaration of ‘bt_crypto_f6’ [-Werror=redundant-decls]
42 | bool bt_crypto_f6(struct bt_crypto *crypto, uint8_t w[16], uint8_t n1[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:42:6: note: previous declaration of ‘bt_crypto_f6’ was here
42 | bool bt_crypto_f6(struct bt_crypto *crypto, uint8_t w[16], uint8_t n1[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:45:6: error: redundant redeclaration of ‘bt_crypto_g2’ [-Werror=redundant-decls]
45 | bool bt_crypto_g2(struct bt_crypto *crypto, uint8_t u[32], uint8_t v[32],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:45:6: note: previous declaration of ‘bt_crypto_g2’ was here
45 | bool bt_crypto_g2(struct bt_crypto *crypto, uint8_t u[32], uint8_t v[32],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:47:6: error: redundant redeclaration of ‘bt_crypto_h6’ [-Werror=redundant-decls]
47 | bool bt_crypto_h6(struct bt_crypto *crypto, const uint8_t w[16],
| ^~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:47:6: note: previous declaration of ‘bt_crypto_h6’ was here
47 | bool bt_crypto_h6(struct bt_crypto *crypto, const uint8_t w[16],
| ^~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:49:6: error: redundant redeclaration of ‘bt_crypto_sign_att’ [-Werror=redundant-decls]
49 | bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:49:6: note: previous declaration of ‘bt_crypto_sign_att’ was here
49 | bool bt_crypto_sign_att(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:52:6: error: redundant redeclaration of ‘bt_crypto_verify_att_sign’ [-Werror=redundant-decls]
52 | bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:52:6: note: previous declaration of ‘bt_crypto_verify_att_sign’ was here
52 | bool bt_crypto_verify_att_sign(struct bt_crypto *crypto, const uint8_t key[16],
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/shared/gatt-db.c:25:
./src/shared/crypto.h:54:6: error: redundant redeclaration of ‘bt_crypto_gatt_hash’ [-Werror=redundant-decls]
54 | bool bt_crypto_gatt_hash(struct bt_crypto *crypto, struct iovec *iov,
| ^~~~~~~~~~~~~~~~~~~
In file included from ./src/shared/gatt-db.h:11,
from src/shared/gatt-db.c:24:
./src/shared/crypto.h:54:6: note: previous declaration of ‘bt_crypto_gatt_hash’ was here
54 | bool bt_crypto_gatt_hash(struct bt_crypto *crypto, struct iovec *iov,
| ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:6808: src/shared/gatt-db.lo] Error 1
make: *** [Makefile:4023: all] Error 2
##############################
Test: MakeCheck - SKIPPED
Output:
checkbuild not success
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Bluez PATCH] gatt: Use the same crypto instance for each device
2021-01-15 12:25 [Bluez PATCH] gatt: Use the same crypto instance for each device Archie Pusaka
2021-01-15 13:37 ` [Bluez] " bluez.test.bot
@ 2021-01-15 18:39 ` Luiz Augusto von Dentz
1 sibling, 0 replies; 3+ messages in thread
From: Luiz Augusto von Dentz @ 2021-01-15 18:39 UTC (permalink / raw)
To: Archie Pusaka
Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka,
Abhishek Pandit-Subedi, Sonny Sasaka
Hi Archie,
On Fri, Jan 15, 2021 at 4:25 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Every time a device is found, BlueZ would need to open 3 socket
> connections for crypto. On a noisy environment where there is a lot
> of advertising devices, we could hit the max opened file limit. As
> a result, connection to the device would fail until the temporary
> devices are removed due to temporary timer timeouts.
>
> This patch store a crypto instance in adapter and reuse the same
> crypto instance for each device.
I think we are better off transforming bt_crypto in a singleton.
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> ---
> Hi BlueZ maintainers,
>
> Is there a reason to have more than one crypto instance? If not,
> we could just have a singleton and a getter get_crypto_instance()
> which would instantiate a crypto object if it doesn't exist, in lieu
> of bt_crypto_new().
>
> android/gatt.c | 2 +-
> peripheral/gatt.c | 4 ++--
> src/adapter.c | 12 ++++++++++++
> src/adapter.h | 2 ++
> src/device.c | 2 +-
> src/gatt-database.c | 2 +-
> src/shared/gatt-db.c | 9 +++++++--
> src/shared/gatt-db.h | 4 +++-
> tools/btgatt-client.c | 2 +-
> tools/btgatt-server.c | 2 +-
> unit/test-gatt.c | 4 ++--
> 11 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index a8a0c488b0..d00c4adf2c 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -7234,7 +7234,7 @@ bool bt_gatt_register(struct ipc *ipc, const bdaddr_t *addr)
> app_connections = queue_new();
> listen_apps = queue_new();
> services_sdp = queue_new();
> - gatt_db = gatt_db_new();
> + gatt_db = gatt_db_new(NULL);
>
> if (!gatt_db) {
> error("gatt: Failed to allocate memory for database");
> diff --git a/peripheral/gatt.c b/peripheral/gatt.c
> index ff1354cb48..1991ec7d4b 100644
> --- a/peripheral/gatt.c
> +++ b/peripheral/gatt.c
> @@ -261,7 +261,7 @@ void gatt_server_start(void)
> return;
> }
>
> - gatt_db = gatt_db_new();
> + gatt_db = gatt_db_new(NULL);
> if (!gatt_db) {
> close(att_fd);
> att_fd = -1;
> @@ -271,7 +271,7 @@ void gatt_server_start(void)
> populate_gap_service(gatt_db);
> populate_devinfo_service(gatt_db);
>
> - gatt_cache = gatt_db_new();
> + gatt_cache = gatt_db_new(NULL);
>
> conn_list = queue_new();
> if (!conn_list) {
> diff --git a/src/adapter.c b/src/adapter.c
> index c1f976323e..794fe1045d 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -46,6 +46,7 @@
> #include "src/shared/queue.h"
> #include "src/shared/att.h"
> #include "src/shared/gatt-db.h"
> +#include "src/shared/crypto.h"
>
> #include "btio/btio.h"
> #include "btd.h"
> @@ -281,6 +282,8 @@ struct btd_adapter {
> bool is_default; /* true if adapter is default one */
>
> bool le_simult_roles_supported;
> +
> + struct bt_crypto *crypto;
> };
>
> typedef enum {
> @@ -5376,6 +5379,8 @@ static void adapter_free(gpointer user_data)
>
> g_slist_free(adapter->connections);
>
> + bt_crypto_unref(adapter->crypto);
> +
> g_free(adapter->path);
> g_free(adapter->name);
> g_free(adapter->short_name);
> @@ -6321,6 +6326,8 @@ static struct btd_adapter *btd_adapter_new(uint16_t index)
>
> adapter->auths = g_queue_new();
>
> + adapter->crypto = bt_crypto_new();
> +
> return btd_adapter_ref(adapter);
> }
>
> @@ -9917,3 +9924,8 @@ bool btd_has_kernel_features(uint32_t features)
> {
> return (kernel_features & features) ? true : false;
> }
> +
> +struct bt_crypto *btd_adapter_get_crypto(struct btd_adapter *adapter)
> +{
> + return adapter->crypto;
> +}
> diff --git a/src/adapter.h b/src/adapter.h
> index 60b5e3bcca..2c7f06493a 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -240,3 +240,5 @@ enum kernel_features {
> };
>
> bool btd_has_kernel_features(uint32_t feature);
> +
> +struct bt_crypto *btd_adapter_get_crypto(struct btd_adapter *adapter);
> diff --git a/src/device.c b/src/device.c
> index af13badfc8..0cf7423cce 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -4047,7 +4047,7 @@ static struct btd_device *device_new(struct btd_adapter *adapter,
>
> device->tx_power = 127;
>
> - device->db = gatt_db_new();
> + device->db = gatt_db_new(btd_adapter_get_crypto(adapter));
> if (!device->db) {
> g_free(device);
> return NULL;
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index d99604826e..2024fdd5d2 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -3623,7 +3623,7 @@ struct btd_gatt_database *btd_gatt_database_new(struct btd_adapter *adapter)
>
> database = new0(struct btd_gatt_database, 1);
> database->adapter = btd_adapter_ref(adapter);
> - database->db = gatt_db_new();
> + database->db = gatt_db_new(btd_adapter_get_crypto(adapter));
> database->records = queue_new();
> database->device_states = queue_new();
> database->apps = queue_new();
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 8bff4d37aa..d616c17e47 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -235,12 +235,17 @@ struct gatt_db *gatt_db_ref(struct gatt_db *db)
> return db;
> }
>
> -struct gatt_db *gatt_db_new(void)
> +struct gatt_db *gatt_db_new(struct bt_crypto *crypto)
> {
> struct gatt_db *db;
>
> db = new0(struct gatt_db, 1);
> - db->crypto = bt_crypto_new();
> +
> + if (crypto)
> + db->crypto = bt_crypto_ref(crypto);
> + else
> + db->crypto = bt_crypto_new();
> +
> db->services = queue_new();
> db->notify_list = queue_new();
> db->next_handle = 0x0001;
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 321a2aba61..1dac1af4fe 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -8,10 +8,12 @@
> *
> */
>
> +#include "src/shared/crypto.h"
> +
> struct gatt_db;
> struct gatt_db_attribute;
>
> -struct gatt_db *gatt_db_new(void);
> +struct gatt_db *gatt_db_new(struct bt_crypto *crypto);
>
> struct gatt_db *gatt_db_ref(struct gatt_db *db);
> void gatt_db_unref(struct gatt_db *db);
> diff --git a/tools/btgatt-client.c b/tools/btgatt-client.c
> index 8c9365aa2c..945e24d6f8 100644
> --- a/tools/btgatt-client.c
> +++ b/tools/btgatt-client.c
> @@ -197,7 +197,7 @@ static struct client *client_create(int fd, uint16_t mtu)
> }
>
> cli->fd = fd;
> - cli->db = gatt_db_new();
> + cli->db = gatt_db_new(NULL);
> if (!cli->db) {
> fprintf(stderr, "Failed to create GATT database\n");
> bt_att_unref(cli->att);
> diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
> index 000145a3db..50004acee5 100644
> --- a/tools/btgatt-server.c
> +++ b/tools/btgatt-server.c
> @@ -569,7 +569,7 @@ static struct server *server_create(int fd, uint16_t mtu, bool hr_visible)
> server->device_name[name_len] = '\0';
>
> server->fd = fd;
> - server->db = gatt_db_new();
> + server->db = gatt_db_new(NULL);
> if (!server->db) {
> fprintf(stderr, "Failed to create GATT database\n");
> goto fail;
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 4aa87d09c4..1916ace6c7 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -674,7 +674,7 @@ static struct context *create_context(uint16_t mtu, gconstpointer data)
> "bt_gatt_server:", NULL);
> break;
> case CLIENT:
> - context->client_db = gatt_db_new();
> + context->client_db = gatt_db_new(NULL);
> g_assert(context->client_db);
>
> context->client = bt_gatt_client_new(context->client_db,
> @@ -1363,7 +1363,7 @@ struct att_handle_spec {
>
> static struct gatt_db *make_db(const struct att_handle_spec *spec)
> {
> - struct gatt_db *db = gatt_db_new();
> + struct gatt_db *db = gatt_db_new(NULL);
> struct gatt_db_attribute *att, *include_att;
> bt_uuid_t uuid;
>
> --
> 2.30.0.296.g2bfb1c46d8-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 3+ messages in thread