* [PATCH] android/gatt: Fix dead code warnings @ 2014-12-19 9:24 Andrei Emeltchenko 2014-12-19 10:07 ` Szymon Janc 2015-01-08 14:19 ` Andrei Emeltchenko 0 siblings, 2 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2014-12-19 9:24 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> gatt_db_attribute_get_permissions() used everywhere without check since those conditions are checked already. --- android/gatt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/android/gatt.c b/android/gatt.c index 0258d91..169f6db 100644 --- a/android/gatt.c +++ b/android/gatt.c @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) - return; + gatt_db_attribute_get_permissions(attrib, &permissions); if (check_device_permissions(dev, cmd[0], permissions)) return; -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-12-19 9:24 [PATCH] android/gatt: Fix dead code warnings Andrei Emeltchenko @ 2014-12-19 10:07 ` Szymon Janc 2014-12-19 10:14 ` Andrei Emeltchenko 2015-01-08 14:19 ` Andrei Emeltchenko 1 sibling, 1 reply; 16+ messages in thread From: Szymon Janc @ 2014-12-19 10:07 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > gatt_db_attribute_get_permissions() used everywhere without check since > those conditions are checked already. > --- > android/gatt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/android/gatt.c b/android/gatt.c > index 0258d91..169f6db 100644 > --- a/android/gatt.c > +++ b/android/gatt.c > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return; > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > - return; > + gatt_db_attribute_get_permissions(attrib, &permissions); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; Wasn't identical patch already reviewed few weeks ago? -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-12-19 10:07 ` Szymon Janc @ 2014-12-19 10:14 ` Andrei Emeltchenko 2014-12-19 10:15 ` Szymon Janc 0 siblings, 1 reply; 16+ messages in thread From: Andrei Emeltchenko @ 2014-12-19 10:14 UTC (permalink / raw) To: Szymon Janc; +Cc: linux-bluetooth Hi Szymon, On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote: > Hi Andrei, > > On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote: > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > those conditions are checked already. > > --- > > android/gatt.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/android/gatt.c b/android/gatt.c > > index 0258d91..169f6db 100644 > > --- a/android/gatt.c > > +++ b/android/gatt.c > > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > > if (!attrib) > > return; > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > - return; > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > return; > > Wasn't identical patch already reviewed few weeks ago? > It is still valid. No better approach was found. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-12-19 10:14 ` Andrei Emeltchenko @ 2014-12-19 10:15 ` Szymon Janc 2014-12-19 10:18 ` [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() Andrei Emeltchenko 2014-12-19 10:22 ` [PATCH] " Andrei Emeltchenko 0 siblings, 2 replies; 16+ messages in thread From: Szymon Janc @ 2014-12-19 10:15 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Friday 19 of December 2014 12:14:50 Andrei Emeltchenko wrote: > Hi Szymon, > > On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote: > > Hi Andrei, > > > > On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote: > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > > those conditions are checked already. > > > --- > > > android/gatt.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/android/gatt.c b/android/gatt.c > > > index 0258d91..169f6db 100644 > > > --- a/android/gatt.c > > > +++ b/android/gatt.c > > > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > > > if (!attrib) > > > return; > > > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > > - return; > > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > > return; > > > > Wasn't identical patch already reviewed few weeks ago? > > > > It is still valid. No better approach was found. I suggested possible solution which you implemented in V2. But you seem to ignored comment from Luiz so this didn't moved on. -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() 2014-12-19 10:15 ` Szymon Janc @ 2014-12-19 10:18 ` Andrei Emeltchenko 2015-01-09 10:10 ` Szymon Janc 2014-12-19 10:22 ` [PATCH] " Andrei Emeltchenko 1 sibling, 1 reply; 16+ messages in thread From: Andrei Emeltchenko @ 2014-12-19 10:18 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> Condition checks inside gatt_db_attribute_get_permissions() do not make sense. Simplify function. --- android/gatt.c | 10 +++++----- src/shared/gatt-db.c | 10 ++-------- src/shared/gatt-db.h | 3 +-- src/shared/gatt-server.c | 20 ++++---------------- 4 files changed, 12 insertions(+), 31 deletions(-) diff --git a/android/gatt.c b/android/gatt.c index 169f6db..6828f2f 100644 --- a/android/gatt.c +++ b/android/gatt.c @@ -4790,7 +4790,7 @@ static void read_requested_attributes(void *data, void *user_data) return; } - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); /* * Check if it is attribute we didn't declare permissions, like service @@ -6311,7 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); if (check_device_permissions(dev, cmd[0], permissions)) return; @@ -6359,7 +6359,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); if (check_device_permissions(dev, cmd[0], permissions)) return; @@ -6430,7 +6430,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return ATT_ECODE_ATTR_NOT_FOUND; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); error = check_device_permissions(dev, cmd[0], permissions); if (error) @@ -6486,7 +6486,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return ATT_ECODE_ATTR_NOT_FOUND; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); error = check_device_permissions(dev, cmd[0], permissions); if (error) diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index bb60904..4b91ae6 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -1301,15 +1301,9 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, return true; } -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, - uint32_t *permissions) +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib) { - if (!attrib || !permissions) - return false; - - *permissions = attrib->permissions; - - return true; + return attrib->permissions; } static void pending_read_result(struct pending_read *p, int err, diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h index e5fe6bb..1379aae 100644 --- a/src/shared/gatt-db.h +++ b/src/shared/gatt-db.h @@ -171,8 +171,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, uint16_t *start_handle, uint16_t *end_handle); -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, - uint32_t *permissions); +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib); typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib, int err, const uint8_t *value, diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c index 00f36fd..f9e412d 100644 --- a/src/shared/gatt-server.c +++ b/src/shared/gatt-server.c @@ -389,10 +389,7 @@ static void process_read_by_type(struct async_read_op *op) return; } - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_UNLIKELY; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); /* * Check for the READ access permission. Encryption, @@ -697,10 +694,7 @@ static void write_cb(uint8_t opcode, const void *pdu, (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); if (!(perm & BT_ATT_PERM_WRITE)) { ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED; @@ -813,10 +807,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode, opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); if (perm && !(perm & BT_ATT_PERM_READ)) { ecode = BT_ATT_ERROR_READ_NOT_PERMITTED; @@ -919,10 +910,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu, util_debug(server->debug_callback, server->debug_data, "Prep Write Req - handle: 0x%04x", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); /* * TODO: The "Prepare Write" request requires security permission checks -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() 2014-12-19 10:18 ` [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() Andrei Emeltchenko @ 2015-01-09 10:10 ` Szymon Janc 2015-01-13 10:09 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Andrei Emeltchenko 0 siblings, 1 reply; 16+ messages in thread From: Szymon Janc @ 2015-01-09 10:10 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Friday 19 of December 2014 12:18:29 Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > Condition checks inside gatt_db_attribute_get_permissions() do not make > sense. Simplify function. In general I'm fine with this change but commit message should be improved. ie. there is API change so this is not just simplification. > --- > android/gatt.c | 10 +++++----- > src/shared/gatt-db.c | 10 ++-------- > src/shared/gatt-db.h | 3 +-- > src/shared/gatt-server.c | 20 ++++---------------- > 4 files changed, 12 insertions(+), 31 deletions(-) > > diff --git a/android/gatt.c b/android/gatt.c > index 169f6db..6828f2f 100644 > --- a/android/gatt.c > +++ b/android/gatt.c > @@ -4790,7 +4790,7 @@ static void read_requested_attributes(void *data, void *user_data) > return; > } > > - gatt_db_attribute_get_permissions(attrib, &permissions); > + permissions = gatt_db_attribute_get_permissions(attrib); > > /* > * Check if it is attribute we didn't declare permissions, like service > @@ -6311,7 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return; > > - gatt_db_attribute_get_permissions(attrib, &permissions); > + permissions = gatt_db_attribute_get_permissions(attrib); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; > @@ -6359,7 +6359,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return; > > - gatt_db_attribute_get_permissions(attrib, &permissions); > + permissions = gatt_db_attribute_get_permissions(attrib); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; > @@ -6430,7 +6430,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return ATT_ECODE_ATTR_NOT_FOUND; > > - gatt_db_attribute_get_permissions(attrib, &permissions); > + permissions = gatt_db_attribute_get_permissions(attrib); > > error = check_device_permissions(dev, cmd[0], permissions); > if (error) > @@ -6486,7 +6486,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return ATT_ECODE_ATTR_NOT_FOUND; > > - gatt_db_attribute_get_permissions(attrib, &permissions); > + permissions = gatt_db_attribute_get_permissions(attrib); > > error = check_device_permissions(dev, cmd[0], permissions); > if (error) > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index bb60904..4b91ae6 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -1301,15 +1301,9 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, > return true; > } > > -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, > - uint32_t *permissions) > +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib) > { > - if (!attrib || !permissions) > - return false; > - > - *permissions = attrib->permissions; > - > - return true; > + return attrib->permissions; > } Let check if attrib is NULL and return 0 in such case. This is to keep convention with other getter-like functions. > > static void pending_read_result(struct pending_read *p, int err, > diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h > index e5fe6bb..1379aae 100644 > --- a/src/shared/gatt-db.h > +++ b/src/shared/gatt-db.h > @@ -171,8 +171,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, > uint16_t *start_handle, > uint16_t *end_handle); > > -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, > - uint32_t *permissions); > +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib); > > typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib, > int err, const uint8_t *value, > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c > index 00f36fd..f9e412d 100644 > --- a/src/shared/gatt-server.c > +++ b/src/shared/gatt-server.c > @@ -389,10 +389,7 @@ static void process_read_by_type(struct async_read_op *op) > return; > } > > - if (!gatt_db_attribute_get_permissions(attr, &perm)) { > - ecode = BT_ATT_ERROR_UNLIKELY; > - goto error; > - } > + perm = gatt_db_attribute_get_permissions(attr); > > /* > * Check for the READ access permission. Encryption, > @@ -697,10 +694,7 @@ static void write_cb(uint8_t opcode, const void *pdu, > (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd", > handle); > > - if (!gatt_db_attribute_get_permissions(attr, &perm)) { > - ecode = BT_ATT_ERROR_INVALID_HANDLE; > - goto error; > - } > + perm = gatt_db_attribute_get_permissions(attr); > > if (!(perm & BT_ATT_PERM_WRITE)) { > ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED; > @@ -813,10 +807,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode, > opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "", > handle); > > - if (!gatt_db_attribute_get_permissions(attr, &perm)) { > - ecode = BT_ATT_ERROR_INVALID_HANDLE; > - goto error; > - } > + perm = gatt_db_attribute_get_permissions(attr); > > if (perm && !(perm & BT_ATT_PERM_READ)) { > ecode = BT_ATT_ERROR_READ_NOT_PERMITTED; > @@ -919,10 +910,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu, > util_debug(server->debug_callback, server->debug_data, > "Prep Write Req - handle: 0x%04x", handle); > > - if (!gatt_db_attribute_get_permissions(attr, &perm)) { > - ecode = BT_ATT_ERROR_INVALID_HANDLE; > - goto error; > - } > + perm = gatt_db_attribute_get_permissions(attr); > > /* > * TODO: The "Prepare Write" request requires security permission checks > -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCHv2 1/2] android/gatt: Fix dead code warnings 2015-01-09 10:10 ` Szymon Janc @ 2015-01-13 10:09 ` Andrei Emeltchenko 2015-01-13 10:09 ` [PATCHv2 2/2] android/gatt: Refactor gatt_db_attribute_get_permissions() Andrei Emeltchenko 2015-01-20 10:48 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Szymon Janc 0 siblings, 2 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2015-01-13 10:09 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> gatt_db_attribute_get_permissions() used everywhere without check since those conditions are checked already. --- android/gatt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/android/gatt.c b/android/gatt.c index b749705..8542f45 100644 --- a/android/gatt.c +++ b/android/gatt.c @@ -6355,8 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) - return; + gatt_db_attribute_get_permissions(attrib, &permissions); if (check_device_permissions(dev, cmd[0], permissions)) return; -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCHv2 2/2] android/gatt: Refactor gatt_db_attribute_get_permissions() 2015-01-13 10:09 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Andrei Emeltchenko @ 2015-01-13 10:09 ` Andrei Emeltchenko 2015-01-20 10:48 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Szymon Janc 1 sibling, 0 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2015-01-13 10:09 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> Refactoring makes code cleaner and consistent with other code like function gatt_db_attribute_get_handle(), etc. Also removes compile warnings related to unneeded checks like check for !permissions and warnings related to dead check for construction below: (!gatt_db_attribute_get_permissions(attr, &perm) those checks are useless since attr is always checked before and check for &perm is really stupid. --- android/gatt.c | 10 +++++----- src/shared/gatt-db.c | 11 ++++------- src/shared/gatt-db.h | 3 +-- src/shared/gatt-server.c | 20 ++++---------------- 4 files changed, 14 insertions(+), 30 deletions(-) diff --git a/android/gatt.c b/android/gatt.c index 8542f45..1d65c2a 100644 --- a/android/gatt.c +++ b/android/gatt.c @@ -4817,7 +4817,7 @@ static void read_requested_attributes(void *data, void *user_data) return; } - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); /* * Check if it is attribute we didn't declare permissions, like service @@ -6355,7 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); if (check_device_permissions(dev, cmd[0], permissions)) return; @@ -6403,7 +6403,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); if (check_device_permissions(dev, cmd[0], permissions)) return; @@ -6474,7 +6474,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return ATT_ECODE_ATTR_NOT_FOUND; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); error = check_device_permissions(dev, cmd[0], permissions); if (error) @@ -6530,7 +6530,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return ATT_ECODE_ATTR_NOT_FOUND; - gatt_db_attribute_get_permissions(attrib, &permissions); + permissions = gatt_db_attribute_get_permissions(attrib); error = check_device_permissions(dev, cmd[0], permissions); if (error) diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index 9a9cadc..2b997f1 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -1345,15 +1345,12 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, return true; } -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, - uint32_t *permissions) +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib) { - if (!attrib || !permissions) - return false; - - *permissions = attrib->permissions; + if (!attrib) + return 0; - return true; + return attrib->permissions; } static void pending_read_result(struct pending_read *p, int err, diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h index 1f4005e..169108a 100644 --- a/src/shared/gatt-db.h +++ b/src/shared/gatt-db.h @@ -181,8 +181,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib, uint16_t *start_handle, uint16_t *end_handle); -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib, - uint32_t *permissions); +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib); typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib, int err, const uint8_t *value, diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c index ed1b274..f12ac47 100644 --- a/src/shared/gatt-server.c +++ b/src/shared/gatt-server.c @@ -391,10 +391,7 @@ static void process_read_by_type(struct async_read_op *op) return; } - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_UNLIKELY; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); /* * Check for the READ access permission. Encryption, @@ -791,10 +788,7 @@ static void write_cb(uint8_t opcode, const void *pdu, (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); if (!(perm & BT_ATT_PERM_WRITE)) { ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED; @@ -907,10 +901,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode, opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); if (perm && !(perm & BT_ATT_PERM_READ)) { ecode = BT_ATT_ERROR_READ_NOT_PERMITTED; @@ -1013,10 +1004,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu, util_debug(server->debug_callback, server->debug_data, "Prep Write Req - handle: 0x%04x", handle); - if (!gatt_db_attribute_get_permissions(attr, &perm)) { - ecode = BT_ATT_ERROR_INVALID_HANDLE; - goto error; - } + perm = gatt_db_attribute_get_permissions(attr); /* * TODO: The "Prepare Write" request requires security permission checks -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCHv2 1/2] android/gatt: Fix dead code warnings 2015-01-13 10:09 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Andrei Emeltchenko 2015-01-13 10:09 ` [PATCHv2 2/2] android/gatt: Refactor gatt_db_attribute_get_permissions() Andrei Emeltchenko @ 2015-01-20 10:48 ` Szymon Janc 1 sibling, 0 replies; 16+ messages in thread From: Szymon Janc @ 2015-01-20 10:48 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Tuesday 13 of January 2015 12:09:18 Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > gatt_db_attribute_get_permissions() used everywhere without check since > those conditions are checked already. > --- > android/gatt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/android/gatt.c b/android/gatt.c > index b749705..8542f45 100644 > --- a/android/gatt.c > +++ b/android/gatt.c > @@ -6355,8 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return; > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > - return; > + gatt_db_attribute_get_permissions(attrib, &permissions); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; > Both patches applied, thanks. -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-12-19 10:15 ` Szymon Janc 2014-12-19 10:18 ` [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() Andrei Emeltchenko @ 2014-12-19 10:22 ` Andrei Emeltchenko 1 sibling, 0 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2014-12-19 10:22 UTC (permalink / raw) To: Szymon Janc; +Cc: linux-bluetooth Hi Szymon, On Fri, Dec 19, 2014 at 11:15:43AM +0100, Szymon Janc wrote: > Hi Andrei, > > On Friday 19 of December 2014 12:14:50 Andrei Emeltchenko wrote: > > Hi Szymon, > > > > On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote: > > > Hi Andrei, > > > > > > On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote: > > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > > > those conditions are checked already. > > > > --- > > > > android/gatt.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/android/gatt.c b/android/gatt.c > > > > index 0258d91..169f6db 100644 > > > > --- a/android/gatt.c > > > > +++ b/android/gatt.c > > > > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > > > > if (!attrib) > > > > return; > > > > > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > > > - return; > > > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > > > return; > > > > > > Wasn't identical patch already reviewed few weeks ago? > > > > > > > It is still valid. No better approach was found. > > I suggested possible solution which you implemented in V2. But you seem to > ignored comment from Luiz so this didn't moved on. I still have the patch, it shall be applied anyway on top of this patch since they are fixing __different__ things. AFAIK Luiz did not propose anything he was just against the patch. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-12-19 9:24 [PATCH] android/gatt: Fix dead code warnings Andrei Emeltchenko 2014-12-19 10:07 ` Szymon Janc @ 2015-01-08 14:19 ` Andrei Emeltchenko 1 sibling, 0 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2015-01-08 14:19 UTC (permalink / raw) To: linux-bluetooth ping On Fri, Dec 19, 2014 at 11:24:40AM +0200, Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > gatt_db_attribute_get_permissions() used everywhere without check since > those conditions are checked already. > --- > android/gatt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/android/gatt.c b/android/gatt.c > index 0258d91..169f6db 100644 > --- a/android/gatt.c > +++ b/android/gatt.c > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, > if (!attrib) > return; > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > - return; > + gatt_db_attribute_get_permissions(attrib, &permissions); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] android/gatt: Fix dead code warnings @ 2014-11-10 13:25 Andrei Emeltchenko 2014-11-10 20:05 ` Szymon Janc 0 siblings, 1 reply; 16+ messages in thread From: Andrei Emeltchenko @ 2014-11-10 13:25 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> gatt_db_attribute_get_permissions() used everywhere without check since those conditions are checked already. --- android/gatt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/android/gatt.c b/android/gatt.c index 086bb94..04101f6 100644 --- a/android/gatt.c +++ b/android/gatt.c @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len, if (!attrib) return; - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) - return; + gatt_db_attribute_get_permissions(attrib, &permissions); if (check_device_permissions(dev, cmd[0], permissions)) return; -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-11-10 13:25 Andrei Emeltchenko @ 2014-11-10 20:05 ` Szymon Janc 2014-11-11 8:22 ` Andrei Emeltchenko 0 siblings, 1 reply; 16+ messages in thread From: Szymon Janc @ 2014-11-10 20:05 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > gatt_db_attribute_get_permissions() used everywhere without check since > those conditions are checked already. It would make sense to put such warning into commit message if you are referring to it in subject. > --- > android/gatt.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/android/gatt.c b/android/gatt.c > index 086bb94..04101f6 100644 > --- a/android/gatt.c > +++ b/android/gatt.c > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, > uint16_t cmd_len, if (!attrib) > return; > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > - return; > + gatt_db_attribute_get_permissions(attrib, &permissions); > > if (check_device_permissions(dev, cmd[0], permissions)) > return; So if we always pass valid pointers to this function, then maybe we should change it definition to something like: uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib); Thoughts? -- BR Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-11-10 20:05 ` Szymon Janc @ 2014-11-11 8:22 ` Andrei Emeltchenko 2014-11-13 13:24 ` Szymon Janc 0 siblings, 1 reply; 16+ messages in thread From: Andrei Emeltchenko @ 2014-11-11 8:22 UTC (permalink / raw) To: Szymon Janc; +Cc: linux-bluetooth Hi Szymon, On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote: > Hi Andrei, > > On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote: > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > those conditions are checked already. > > It would make sense to put such warning into commit message if you are > referring to it in subject. > > > --- > > android/gatt.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/android/gatt.c b/android/gatt.c > > index 086bb94..04101f6 100644 > > --- a/android/gatt.c > > +++ b/android/gatt.c > > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, > > uint16_t cmd_len, if (!attrib) > > return; > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > - return; > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > return; > > So if we always pass valid pointers to this function, then maybe we should > change it definition to something like: > > uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib); > Do we really need a helper which just does reference? Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-11-11 8:22 ` Andrei Emeltchenko @ 2014-11-13 13:24 ` Szymon Janc 2014-11-13 13:33 ` Andrei Emeltchenko 0 siblings, 1 reply; 16+ messages in thread From: Szymon Janc @ 2014-11-13 13:24 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Tuesday 11 of November 2014 10:22:43 Andrei Emeltchenko wrote: > Hi Szymon, > > On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote: > > Hi Andrei, > > > > On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote: > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > > those conditions are checked already. > > > > It would make sense to put such warning into commit message if you are > > referring to it in subject. > > > > > --- > > > android/gatt.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/android/gatt.c b/android/gatt.c > > > index 086bb94..04101f6 100644 > > > --- a/android/gatt.c > > > +++ b/android/gatt.c > > > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, > > > uint16_t cmd_len, if (!attrib) > > > return; > > > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > > - return; > > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > > return; > > > > So if we always pass valid pointers to this function, then maybe we should > > change it definition to something like: > > > > uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib); > > > > Do we really need a helper which just does reference? gatt_db_attribute is private structure. -- Best regards, Szymon Janc ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] android/gatt: Fix dead code warnings 2014-11-13 13:24 ` Szymon Janc @ 2014-11-13 13:33 ` Andrei Emeltchenko 0 siblings, 0 replies; 16+ messages in thread From: Andrei Emeltchenko @ 2014-11-13 13:33 UTC (permalink / raw) To: Szymon Janc; +Cc: linux-bluetooth Hi Szymon, On Thu, Nov 13, 2014 at 02:24:40PM +0100, Szymon Janc wrote: > Hi Andrei, > > On Tuesday 11 of November 2014 10:22:43 Andrei Emeltchenko wrote: > > Hi Szymon, > > > > On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote: > > > Hi Andrei, > > > > > > On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote: > > > > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > > > > > > > gatt_db_attribute_get_permissions() used everywhere without check since > > > > those conditions are checked already. > > > > > > It would make sense to put such warning into commit message if you are > > > referring to it in subject. > > > > > > > --- > > > > android/gatt.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/android/gatt.c b/android/gatt.c > > > > index 086bb94..04101f6 100644 > > > > --- a/android/gatt.c > > > > +++ b/android/gatt.c > > > > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, > > > > uint16_t cmd_len, if (!attrib) > > > > return; > > > > > > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions)) > > > > - return; > > > > + gatt_db_attribute_get_permissions(attrib, &permissions); > > > > > > > > if (check_device_permissions(dev, cmd[0], permissions)) > > > > return; > > > > > > So if we always pass valid pointers to this function, then maybe we should > > > change it definition to something like: > > > > > > uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib); > > > > > > > Do we really need a helper which just does reference? > > gatt_db_attribute is private structure. Ah, true. I will rebase the patch following you suggestion. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-01-20 10:48 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-19 9:24 [PATCH] android/gatt: Fix dead code warnings Andrei Emeltchenko 2014-12-19 10:07 ` Szymon Janc 2014-12-19 10:14 ` Andrei Emeltchenko 2014-12-19 10:15 ` Szymon Janc 2014-12-19 10:18 ` [PATCH] android/gatt: Simplify gatt_db_attribute_get_permissions() Andrei Emeltchenko 2015-01-09 10:10 ` Szymon Janc 2015-01-13 10:09 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Andrei Emeltchenko 2015-01-13 10:09 ` [PATCHv2 2/2] android/gatt: Refactor gatt_db_attribute_get_permissions() Andrei Emeltchenko 2015-01-20 10:48 ` [PATCHv2 1/2] android/gatt: Fix dead code warnings Szymon Janc 2014-12-19 10:22 ` [PATCH] " Andrei Emeltchenko 2015-01-08 14:19 ` Andrei Emeltchenko -- strict thread matches above, loose matches on Subject: below -- 2014-11-10 13:25 Andrei Emeltchenko 2014-11-10 20:05 ` Szymon Janc 2014-11-11 8:22 ` Andrei Emeltchenko 2014-11-13 13:24 ` Szymon Janc 2014-11-13 13:33 ` Andrei Emeltchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).