2015-02-26 15:20:19

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

From: Andrei Emeltchenko <[email protected]>

Makes code identical to android/avdtp
---
profiles/audio/avdtp.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 896f101..15b611f 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -717,6 +717,9 @@ static void stream_free(void *data)
if (stream->timer)
g_source_remove(stream->timer);

+ if (stream->start_timer > 0)
+ g_source_remove(stream->start_timer);
+
if (stream->io)
close_stream(stream);

--
2.1.0



2015-02-27 17:23:32

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

Hi Luiz,

>>> On Thu, Feb 26, 2015 at 5:20 PM, Andrei Emeltchenko
>>> <[email protected]> wrote:
>>>> From: Andrei Emeltchenko <[email protected]>
>>>>
>>>> Makes code identical to android/avdtp
>>>> ---
>>>> profiles/audio/avdtp.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
>>>> index 896f101..15b611f 100644
>>>> --- a/profiles/audio/avdtp.c
>>>> +++ b/profiles/audio/avdtp.c
>>>> @@ -717,6 +717,9 @@ static void stream_free(void *data)
>>>> if (stream->timer)
>>>> g_source_remove(stream->timer);
>>>>
>>>> + if (stream->start_timer > 0)
>>>> + g_source_remove(stream->start_timer);
>>>> +
>>>> if (stream->io)
>>>> close_stream(stream);
>>>>
>>>> --
>>>> 2.1.0
>>>
>>> I will take a at this over the weekend, I hope I can make the move
>>> without having to redo the patches otherwise the git history will be
>>> lost e.g:
>>
>> You are talking about loosing "git blame" stuff since history does not get
>> lost. In this situation we have to loose either profiles/ or android/ "git
>> blame" history and I am not sure which one is more important.
>
> git mv should preserve the history, and that what Im going for.

you do realize that there is no such thing as git mv in the end. That is just a more convenient helper for mv itself and git add. In the end that magic all happens with git's rename detection. The history will never be lost. It is just a bit harder to find it :)

Regards

Marcel


2015-02-27 15:21:24

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

Hi Luiz,

On Fri, Feb 27, 2015 at 05:05:52PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Fri, Feb 27, 2015 at 4:42 PM, Andrei Emeltchenko
> <[email protected]> wrote:
> > Hi Luiz,
> >
> > On Fri, Feb 27, 2015 at 04:27:45PM +0200, Luiz Augusto von Dentz wrote:
> >> Hi Andrei,
> >>
> >> On Thu, Feb 26, 2015 at 5:20 PM, Andrei Emeltchenko
> >> <[email protected]> wrote:
> >> > From: Andrei Emeltchenko <[email protected]>
> >> >
> >> > Makes code identical to android/avdtp
> >> > ---
> >> > profiles/audio/avdtp.c | 3 +++
> >> > 1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> >> > index 896f101..15b611f 100644
> >> > --- a/profiles/audio/avdtp.c
> >> > +++ b/profiles/audio/avdtp.c
> >> > @@ -717,6 +717,9 @@ static void stream_free(void *data)
> >> > if (stream->timer)
> >> > g_source_remove(stream->timer);
> >> >
> >> > + if (stream->start_timer > 0)
> >> > + g_source_remove(stream->start_timer);
> >> > +
> >> > if (stream->io)
> >> > close_stream(stream);
> >> >
> >> > --
> >> > 2.1.0
> >>
> >> I will take a at this over the weekend, I hope I can make the move
> >> without having to redo the patches otherwise the git history will be
> >> lost e.g:
> >
> > You are talking about loosing "git blame" stuff since history does not get
> > lost. In this situation we have to loose either profiles/ or android/ "git
> > blame" history and I am not sure which one is more important.
>
> git mv should preserve the history, and that what Im going for.

This is what I am talking about.

I suppose you move android/avdtp.c over profiles/audio/avdtp.c. So you
loose history for the latter.

Best regards
Andrei Emeltchenko

2015-02-27 15:05:52

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

Hi Andrei,

On Fri, Feb 27, 2015 at 4:42 PM, Andrei Emeltchenko
<[email protected]> wrote:
> Hi Luiz,
>
> On Fri, Feb 27, 2015 at 04:27:45PM +0200, Luiz Augusto von Dentz wrote:
>> Hi Andrei,
>>
>> On Thu, Feb 26, 2015 at 5:20 PM, Andrei Emeltchenko
>> <[email protected]> wrote:
>> > From: Andrei Emeltchenko <[email protected]>
>> >
>> > Makes code identical to android/avdtp
>> > ---
>> > profiles/audio/avdtp.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
>> > index 896f101..15b611f 100644
>> > --- a/profiles/audio/avdtp.c
>> > +++ b/profiles/audio/avdtp.c
>> > @@ -717,6 +717,9 @@ static void stream_free(void *data)
>> > if (stream->timer)
>> > g_source_remove(stream->timer);
>> >
>> > + if (stream->start_timer > 0)
>> > + g_source_remove(stream->start_timer);
>> > +
>> > if (stream->io)
>> > close_stream(stream);
>> >
>> > --
>> > 2.1.0
>>
>> I will take a at this over the weekend, I hope I can make the move
>> without having to redo the patches otherwise the git history will be
>> lost e.g:
>
> You are talking about loosing "git blame" stuff since history does not get
> lost. In this situation we have to loose either profiles/ or android/ "git
> blame" history and I am not sure which one is more important.

git mv should preserve the history, and that what Im going for.

--
Luiz Augusto von Dentz

2015-02-27 14:42:17

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

Hi Luiz,

On Fri, Feb 27, 2015 at 04:27:45PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Thu, Feb 26, 2015 at 5:20 PM, Andrei Emeltchenko
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > Makes code identical to android/avdtp
> > ---
> > profiles/audio/avdtp.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> > index 896f101..15b611f 100644
> > --- a/profiles/audio/avdtp.c
> > +++ b/profiles/audio/avdtp.c
> > @@ -717,6 +717,9 @@ static void stream_free(void *data)
> > if (stream->timer)
> > g_source_remove(stream->timer);
> >
> > + if (stream->start_timer > 0)
> > + g_source_remove(stream->start_timer);
> > +
> > if (stream->io)
> > close_stream(stream);
> >
> > --
> > 2.1.0
>
> I will take a at this over the weekend, I hope I can make the move
> without having to redo the patches otherwise the git history will be
> lost e.g:

You are talking about loosing "git blame" stuff since history does not get
lost. In this situation we have to loose either profiles/ or android/ "git
blame" history and I am not sure which one is more important.

Best regards
Andrei Emeltchenko

>
> android/AVDTP: Fix not removing start_timer source
>
> It is possible that stream->start_timer can be set on stream_free which
> should then should take care of remove it properly otherwise it can
> trigger which would very likely cause a crash.
>
> Thanks to Hannu Mallat <[email protected]> for reporting it.
>
>
> --
> Luiz Augusto von Dentz

2015-02-27 14:27:45

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH 1/4] audio/avdtp: Remove start timer on stream_free()

Hi Andrei,

On Thu, Feb 26, 2015 at 5:20 PM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Makes code identical to android/avdtp
> ---
> profiles/audio/avdtp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
> index 896f101..15b611f 100644
> --- a/profiles/audio/avdtp.c
> +++ b/profiles/audio/avdtp.c
> @@ -717,6 +717,9 @@ static void stream_free(void *data)
> if (stream->timer)
> g_source_remove(stream->timer);
>
> + if (stream->start_timer > 0)
> + g_source_remove(stream->start_timer);
> +
> if (stream->io)
> close_stream(stream);
>
> --
> 2.1.0

I will take a at this over the weekend, I hope I can make the move
without having to redo the patches otherwise the git history will be
lost e.g:

android/AVDTP: Fix not removing start_timer source

It is possible that stream->start_timer can be set on stream_free which
should then should take care of remove it properly otherwise it can
trigger which would very likely cause a crash.

Thanks to Hannu Mallat <[email protected]> for reporting it.


--
Luiz Augusto von Dentz

2015-02-26 15:20:20

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 2/4] audio/avdtp: Add check for callback before run it

From: Andrei Emeltchenko <[email protected]>

---
profiles/audio/avdtp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 15b611f..3c4e9b1 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1035,15 +1035,17 @@ static void finalize_discovery(struct avdtp *session, int err)
if (!discover)
return;

+ session->discover = NULL;
+
avdtp_error_init(&avdtp_err, AVDTP_ERRNO, err);

if (discover->id > 0)
g_source_remove(discover->id);

- discover->cb(session, session->seps, err ? &avdtp_err : NULL,
+ if (discover->cb)
+ discover->cb(session, session->seps, err ? &avdtp_err : NULL,
discover->user_data);
g_free(discover);
- session->discover = NULL;
}

static void release_stream(struct avdtp_stream *stream, struct avdtp *session)
--
2.1.0


2015-02-26 15:20:22

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 4/4] audio/avdtp: Fix style issues

From: Andrei Emeltchenko <[email protected]>

Makes code in profiles/ and android/ identical in style.
---
profiles/audio/avdtp.c | 38 ++++++++++++++++++++++++--------------
profiles/audio/avdtp.h | 3 ++-
2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 1cad010..a8444ed 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1502,11 +1502,15 @@ static gboolean avdtp_setconf_cmd(struct avdtp *session, uint8_t transaction,
&stream->codec,
&stream->delay_reporting);

- /* Verify that the Media Transport capability's length = 0. Reject otherwise */
+ /*
+ * Verify that the Media Transport capability's length = 0.
+ * Reject otherwise
+ */
for (l = stream->caps; l != NULL; l = g_slist_next(l)) {
struct avdtp_service_capability *cap = l->data;

- if (cap->category == AVDTP_MEDIA_TRANSPORT && cap->length != 0) {
+ if (cap->category == AVDTP_MEDIA_TRANSPORT &&
+ cap->length != 0) {
err = AVDTP_BAD_MEDIA_TRANSPORT_FORMAT;
goto failed_stream;
}
@@ -1578,7 +1582,7 @@ static gboolean avdtp_getconf_cmd(struct avdtp *session, uint8_t transaction,
goto failed;
}

- for (l = sep->stream->caps, rsp_size = 0; l != NULL; l = g_slist_next(l)) {
+ for (l = sep->stream->caps, rsp_size = 0; l; l = g_slist_next(l)) {
struct avdtp_service_capability *cap = l->data;

if (rsp_size + cap->length + 2 > (int) sizeof(buf))
@@ -2042,7 +2046,8 @@ static enum avdtp_parse_result avdtp_parse_data(struct avdtp *session,
switch (header->packet_type) {
case AVDTP_PKT_TYPE_SINGLE:
if (size < sizeof(*single)) {
- error("Received too small single packet (%zu bytes)", size);
+ error("Received too small single packet (%zu bytes)",
+ size);
return PARSE_ERROR;
}
if (session->in.active) {
@@ -2063,7 +2068,8 @@ static enum avdtp_parse_result avdtp_parse_data(struct avdtp *session,
break;
case AVDTP_PKT_TYPE_START:
if (size < sizeof(*start)) {
- error("Received too small start packet (%zu bytes)", size);
+ error("Received too small start packet (%zu bytes)",
+ size);
return PARSE_ERROR;
}
if (session->in.active) {
@@ -2107,7 +2113,8 @@ static enum avdtp_parse_result avdtp_parse_data(struct avdtp *session,
break;
case AVDTP_PKT_TYPE_END:
if (size < sizeof(struct avdtp_continue_header)) {
- error("Received too small end packet (%zu bytes)", size);
+ error("Received too small end packet (%zu bytes)",
+ size);
return PARSE_ERROR;
}
if (!session->in.active) {
@@ -2622,7 +2629,7 @@ static int cancel_request(struct avdtp *session, int err)
error("SetConfiguration: %s (%d)", strerror(err), err);
if (lsep && lsep->cfm && lsep->cfm->set_configuration)
lsep->cfm->set_configuration(session, lsep, stream,
- &averr, lsep->user_data);
+ &averr, lsep->user_data);
goto failed;
case AVDTP_DISCOVER:
error("Discover: %s (%d)", strerror(err), err);
@@ -2821,9 +2828,9 @@ static gboolean avdtp_get_capabilities_resp(struct avdtp *session,
}

static gboolean avdtp_set_configuration_resp(struct avdtp *session,
- struct avdtp_stream *stream,
- struct avdtp_single_header *resp,
- int size)
+ struct avdtp_stream *stream,
+ struct avdtp_single_header *resp,
+ int size)
{
struct avdtp_local_sep *sep = stream->lsep;

@@ -2838,7 +2845,8 @@ static gboolean avdtp_set_configuration_resp(struct avdtp *session,

static gboolean avdtp_reconfigure_resp(struct avdtp *session,
struct avdtp_stream *stream,
- struct avdtp_single_header *resp, int size)
+ struct avdtp_single_header *resp,
+ int size)
{
return TRUE;
}
@@ -2924,7 +2932,8 @@ static gboolean avdtp_delay_report_resp(struct avdtp *session,
struct avdtp_local_sep *sep = stream->lsep;

if (sep->cfm && sep->cfm->delay_report)
- sep->cfm->delay_report(session, sep, stream, NULL, sep->user_data);
+ sep->cfm->delay_report(session, sep, stream, NULL,
+ sep->user_data);

return TRUE;
}
@@ -3280,7 +3289,8 @@ struct avdtp_service_capability *avdtp_get_codec(struct avdtp_remote_sep *sep)
}

struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
- void *data, int length)
+ const void *data,
+ int length)
{
struct avdtp_service_capability *cap;

@@ -3752,7 +3762,7 @@ const char *avdtp_strerror(struct avdtp_error *err)
if (err->category == AVDTP_ERRNO)
return strerror(err->err.posix_errno);

- switch(err->err.error_code) {
+ switch (err->err.error_code) {
case AVDTP_BAD_HEADER_FORMAT:
return "Bad Header Format";
case AVDTP_BAD_LENGTH:
diff --git a/profiles/audio/avdtp.h b/profiles/audio/avdtp.h
index a68dc4d..88e8cb4 100644
--- a/profiles/audio/avdtp.h
+++ b/profiles/audio/avdtp.h
@@ -218,7 +218,8 @@ void avdtp_unref(struct avdtp *session);
struct avdtp *avdtp_ref(struct avdtp *session);

struct avdtp_service_capability *avdtp_service_cap_new(uint8_t category,
- void *data, int size);
+ const void *data,
+ int size);

struct avdtp_service_capability *avdtp_get_codec(struct avdtp_remote_sep *sep);

--
2.1.0


2015-02-26 15:20:21

by Andrei Emeltchenko

[permalink] [raw]
Subject: [PATCH 3/4] audio/avdtp: Add AVDTP_STATE_OPEN to allowed state

From: Andrei Emeltchenko <[email protected]>

In delay report handling add AVDTP_STATE_OPEN to good states.
---
profiles/audio/avdtp.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 3c4e9b1..1cad010 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1948,10 +1948,17 @@ static gboolean avdtp_delayreport_cmd(struct avdtp *session,

stream = sep->stream;

- if (sep->state != AVDTP_STATE_CONFIGURED &&
- sep->state != AVDTP_STATE_STREAMING) {
+ switch (sep->state) {
+ case AVDTP_STATE_IDLE:
+ case AVDTP_STATE_ABORTING:
+ case AVDTP_STATE_CLOSING:
err = AVDTP_BAD_STATE;
goto failed;
+ case AVDTP_STATE_CONFIGURED:
+ case AVDTP_STATE_OPEN:
+ case AVDTP_STATE_STREAMING:
+ default:
+ break;
}

stream->delay = ntohs(req->delay);
--
2.1.0