* [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: 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
* 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-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
* 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-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-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
* [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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.