2014-01-10 14:56:20

by Andrei Emeltchenko

Subject: [PATCH] avrcp: Remove dead code

From: Andrei Emeltchenko <[email protected]>

session->target cannot be NULL since it is already checked 11 lines
above:
...
if (session == NULL || session->target == NULL)
return -ENOTCONN;
...
---
profiles/audio/avrcp.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 8d4309a..197959f 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)

DBG("volume=%u", volume);

- if (session->target) {
- pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
- pdu->params[0] = volume;
- pdu->params_len = htons(1);
+ pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
+ pdu->params[0] = volume;
+ pdu->params_len = htons(1);

- return avctp_send_vendordep_req(session->conn,
+ return avctp_send_vendordep_req(session->conn,
AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
buf, sizeof(buf),
avrcp_handle_set_volume, session);
- } else if (session->registered_events &
- (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
- uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
- pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
- pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
- pdu->params[1] = volume;
- pdu->params_len = htons(2);
-
- return avctp_send_vendordep(session->conn,
- session->transaction_events[id],
- AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
- buf, sizeof(buf));
- }
-
- return 0;
}

static int avrcp_connect(struct btd_service *service)
--
1.8.3.2

2014-01-13 10:55:53

by Andrei Emeltchenko

Subject: Re: [PATCH] avrcp: Remove dead code

Hi Luiz,

On Mon, Jan 13, 2014 at 11:08:49AM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Fri, Jan 10, 2014 at 4:56 PM, Andrei Emeltchenko
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > session->target cannot be NULL since it is already checked 11 lines
> > above:
> > ...
> > if (session == NULL || session->target == NULL)
> > return -ENOTCONN;
> > ...
> > ---
> > profiles/audio/avrcp.c | 24 ++++--------------------
> > 1 file changed, 4 insertions(+), 20 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 8d4309a..197959f 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)
> >
> > DBG("volume=%u", volume);
> >
> > - if (session->target) {
> > - pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> > - pdu->params[0] = volume;
> > - pdu->params_len = htons(1);
> > + pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> > + pdu->params[0] = volume;
> > + pdu->params_len = htons(1);
> >
> > - return avctp_send_vendordep_req(session->conn,
> > + return avctp_send_vendordep_req(session->conn,
> > AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
> > buf, sizeof(buf),
> > avrcp_handle_set_volume, session);
> > - } else if (session->registered_events &
> > - (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
> > - uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
> > - pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
> > - pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
> > - pdu->params[1] = volume;
> > - pdu->params_len = htons(2);
> > -
> > - return avctp_send_vendordep(session->conn,
> > - session->transaction_events[id],
> > - AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
> > - buf, sizeof(buf));
> > - }
>
> The above code is necessary to notify volume changes when acting as
> controller (session->control is set), so your fix is not good either
> and we should depend on session->target on the first place because
> both session->control and session->target can be set.


So did you make a proper fix, since otherwise this is dead code?

Best regards
Andrei Emeltchenko

2014-01-13 09:08:49

by Luiz Augusto von Dentz

Subject: Re: [PATCH] avrcp: Remove dead code

Hi Andrei,

On Fri, Jan 10, 2014 at 4:56 PM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> session->target cannot be NULL since it is already checked 11 lines
> above:
> ...
> if (session == NULL || session->target == NULL)
> return -ENOTCONN;
> ...
> ---
> profiles/audio/avrcp.c | 24 ++++--------------------
> 1 file changed, 4 insertions(+), 20 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 8d4309a..197959f 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -3722,30 +3722,14 @@ int avrcp_set_volume(struct btd_device *dev, uint8_t volume)
>
> DBG("volume=%u", volume);
>
> - if (session->target) {
> - pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> - pdu->params[0] = volume;
> - pdu->params_len = htons(1);
> + pdu->pdu_id = AVRCP_SET_ABSOLUTE_VOLUME;
> + pdu->params[0] = volume;
> + pdu->params_len = htons(1);
>
> - return avctp_send_vendordep_req(session->conn,
> + return avctp_send_vendordep_req(session->conn,
> AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
> buf, sizeof(buf),
> avrcp_handle_set_volume, session);
> - } else if (session->registered_events &
> - (1 << AVRCP_EVENT_VOLUME_CHANGED)) {
> - uint8_t id = AVRCP_EVENT_VOLUME_CHANGED;
> - pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
> - pdu->params[0] = AVRCP_EVENT_VOLUME_CHANGED;
> - pdu->params[1] = volume;
> - pdu->params_len = htons(2);
> -
> - return avctp_send_vendordep(session->conn,
> - session->transaction_events[id],
> - AVC_CTYPE_CHANGED, AVC_SUBUNIT_PANEL,
> - buf, sizeof(buf));
> - }

The above code is necessary to notify volume changes when acting as
controller (session->control is set), so your fix is not good either
and we should depend on session->target on the first place because
both session->control and session->target can be set.