[PATCH 1/7] obexd/client : Handle the MAP Extended Event Report 1.1

[PATCH 1/7] obexd/client : Handle the MAP Extended Event Report 1.1

[Gowtham Anandha Babu]

Changes made to handle the Extended Event Report 1.1

1) In SDP, the MAP supported featured bit is updated.
2) In map-event, corresponding new event type and the new attribute introduced in Event report 1.1 is added.
3) In mns corresponding handlers are added.

First two patches related to MAP 1.2 implementation.
Rest are all simple issues.
---
 lib/sdp.h                |  2 +-
 obexd/client/map-event.h |  7 ++++++-
 obexd/client/mns.c       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/lib/sdp.h b/lib/sdp.h
index cc10e9f..76f61e1 100644
--- a/lib/sdp.h
+++ b/lib/sdp.h
@@ -306,7 +306,7 @@ extern "C" {
 #define SDP_ATTR_MAS_INSTANCE_ID		0x0315
 #define SDP_ATTR_SUPPORTED_MESSAGE_TYPES	0x0316
 #define SDP_ATTR_PBAP_SUPPORTED_FEATURES	0x0317
-#define SDP_ATTR_MAP_SUPPORTED_FEATURES		0x0317
+#define SDP_ATTR_MAP_SUPPORTED_FEATURES		0x031f
 
 #define SDP_ATTR_SPECIFICATION_ID		0x0200
 #define SDP_ATTR_VENDOR_ID			0x0201
diff --git a/obexd/client/map-event.h b/obexd/client/map-event.h
index ba5d5d2..99cb0c2 100644
--- a/obexd/client/map-event.h
+++ b/obexd/client/map-event.h
@@ -32,7 +32,8 @@ enum map_event_type {
 	MAP_ET_MEMORY_FULL,
 	MAP_ET_MEMORY_AVAILABLE,
 	MAP_ET_MESSAGE_DELETED,
-	MAP_ET_MESSAGE_SHIFT
+	MAP_ET_MESSAGE_SHIFT,
+	MAP_ET_READ_STATUS_CHANGED
 };
 
 struct map_event {
@@ -41,6 +42,10 @@ struct map_event {
 	char *folder;
 	char *old_folder;
 	char *msg_type;
+	char *datetime;
+	char *subject;
+	char *sender_name;
+	char *priority;
 };
 
 /* Handle notification in map client.
diff --git a/obexd/client/mns.c b/obexd/client/mns.c
index d638886..8087933 100644
--- a/obexd/client/mns.c
+++ b/obexd/client/mns.c
@@ -180,6 +180,8 @@ static void parse_event_report_type(struct map_event *event, const char *value)
 		event->type = MAP_ET_MESSAGE_DELETED;
 	else if (!g_ascii_strcasecmp(value, "MessageShift"))
 		event->type = MAP_ET_MESSAGE_SHIFT;
+	else if (!g_ascii_strcasecmp(value, "ReadStatusChanged"))
+		event->type = MAP_ET_READ_STATUS_CHANGED;
 }
 
 static void parse_event_report_handle(struct map_event *event,
@@ -218,6 +220,30 @@ static void parse_event_report_msg_type(struct map_event *event,
 	event->msg_type = g_strdup(value);
 }
 
+static void parse_event_report_date_time(struct map_event *event,
+                                                        const char *value)
+{
+	event->datetime = g_strdup(value);
+}
+
+static void parse_event_report_subject(struct map_event *event,
+                                                        const char *value)
+{
+	event->subject = g_strdup(value);
+}
+
+static void parse_event_report_sender_name(struct map_event *event,
+                                                        const char *value)
+{
+	event->sender_name = g_strdup(value);
+}
+
+static void parse_event_report_priority(struct map_event *event,
+                                                        const char *value)
+{
+	event->priority = g_strdup(value);
+}
+
 static struct map_event_report_parser {
 	const char *name;
 	void (*func) (struct map_event *event, const char *value);
@@ -227,6 +253,10 @@ static struct map_event_report_parser {
 		{ "folder", parse_event_report_folder },
 		{ "old_folder", parse_event_report_old_folder },
 		{ "msg_type", parse_event_report_msg_type },
+		{ "datetime", parse_event_report_date_time },
+		{ "subject", parse_event_report_subject },
+		{ "sender_name", parse_event_report_sender_name },
+		{ "priority", parse_event_report_priority },
 		{ }
 };
 
-- 
1.9.1

[PATCH 2/7] obexd/client/map : Handle MAP ReadStatusChanged event type

[Gowtham Anandha Babu]

Adds ReadStatusChanged MCE event type handling in map_handle_notification()
---
 obexd/client/map.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/obexd/client/map.c b/obexd/client/map.c
index 520e492..a505707 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1872,6 +1872,25 @@ static void map_handle_status_changed(struct map_data *map,
 								"Status");
 }
 
+static void map_handle_read_status_changed(struct map_data *map,
+                                                        struct map_event *event)
+{
+	struct map_msg *msg;
+
+	msg = g_hash_table_lookup(map->messages, &event->handle);
+	if (msg == NULL)
+		return;
+
+	/* In MAP V 1.2 specification : ReadStatusChanged event didn't have  clear explaination.
+	So as of now it will set the message read status as "yes" if it was "no" already
+	and the other way round. This implementation may change once we get 'read' attribute in the event report. */
+
+	if(msg->flags & MAP_MSG_FLAG_READ)
+		parse_read(msg,"no");
+	else
+		parse_read(msg,"yes");
+}
+
 static void map_handle_folder_changed(struct map_data *map,
 							struct map_event *event,
 							const char *folder)
@@ -1927,6 +1946,9 @@ static void map_handle_notification(struct map_event *event, void *user_data)
 	case MAP_ET_MESSAGE_SHIFT:
 		map_handle_folder_changed(map, event, event->folder);
 		break;
+	case MAP_ET_READ_STATUS_CHANGED:
+		map_handle_read_status_changed(map, event);
+		break;
 	default:
 		break;
 	}
-- 
1.9.1

[PATCH 3/7] tools/btsnoop : Fix variable reassigning issue

[Gowtham Anandha Babu]

The Variable 'written' is reassigned a value before the old one has been used.
The below on fix the same.
---
 tools/btsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/btsnoop.c b/tools/btsnoop.c
index 6ca62d2..86f4691 100644
--- a/tools/btsnoop.c
+++ b/tools/btsnoop.c
@@ -211,7 +211,7 @@ next_packet:
 		goto next_packet;
 	}
 
-	written = input_pkt[select_input].size = htobe32(toread - 1);
+	input_pkt[select_input].size = htobe32(toread - 1);
 	written = input_pkt[select_input].len = htobe32(toread - 1);
 
 	switch (buf[0]) {
-- 
1.9.1

[PATCH 4/7] tools/csr_usb : Fix Resource leak: file

[Gowtham Anandha Babu]

Handles resource leak.
---
 tools/csr_usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/csr_usb.c b/tools/csr_usb.c
index 5fb6bdc..a1d7324 100644
--- a/tools/csr_usb.c
+++ b/tools/csr_usb.c
@@ -80,9 +80,12 @@ static int read_value(const char *name, const char *attr, const char *format)
 		return -1;
 
 	n = fscanf(file, format, &value);
-	if (n != 1)
+	if (n != 1) {
+		fclose(file);
 		return -1;
+	}
 
+	fclose(file);
 	return value;
 }
 
-- 
1.9.1

[PATCH 5/7] tools/seq2bseq : Fix the same expression issue in if condition

[Gowtham Anandha Babu]

Fix the usage of same expression on both sides of '||' in if
---
 tools/seq2bseq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/seq2bseq.c b/tools/seq2bseq.c
index 7657a57..521d20e 100644
--- a/tools/seq2bseq.c
+++ b/tools/seq2bseq.c
@@ -40,7 +40,7 @@ static int convert_line(int fd, const char *line)
 	char str[3];
 	unsigned char val;
 
-	if (line[0] == '*' || line[0] == '\r' || line[0] == '\r')
+	if (line[0] == '*' || line[0] == '\r')
 		return 0;
 
 	while (1) {
-- 
1.9.1

[PATCH 6/7] tools/hciattach : Fix syntax error

[Gowtham Anandha Babu]

Fix the syntax error.
---
 tools/hciattach.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hciattach.c b/tools/hciattach.c
index 1904ac5..db68b51 100644
--- a/tools/hciattach.c
+++ b/tools/hciattach.c
@@ -128,7 +128,7 @@ int uart_speed(int s)
 		return B3500000;
 #endif
 #ifdef B3710000
-	case 3710000
+	case 3710000:
 		return B3710000;
 #endif
 #ifdef B4000000
-- 
1.9.1

[PATCH 7/7] android/hal-utils.c : Fix null pointer dereference

[Gowtham Anandha Babu]

Handles the possible null pointer dereference: bd_addr by checking it against null.
---
 android/hal-utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/android/hal-utils.c b/android/hal-utils.c
index ceefefc..64ab5a1 100644
--- a/android/hal-utils.c
+++ b/android/hal-utils.c
@@ -166,11 +166,13 @@ int int2str_findstr(const char *str, const struct int2str m[])
  */
 const char *bt_bdaddr_t2str(const bt_bdaddr_t *bd_addr, char *buf)
 {
-	const uint8_t *p = bd_addr->address;
+	const uint8_t *p;
 
 	if (!bd_addr)
 		return strcpy(buf, "NULL");
 
+	p = bd_addr->address;
+
 	snprintf(buf, MAX_ADDR_STR_LEN, "%02x:%02x:%02x:%02x:%02x:%02x",
 					p[0], p[1], p[2], p[3], p[4], p[5]);
 
-- 
1.9.1

Re: [PATCH 1/7] obexd/client : Handle the MAP Extended Event Report 1.1

[Johan Hedberg]

Hi,

On Thu, Sep 11, 2014, Gowtham Anandha Babu wrote:
>  struct map_event {
> @@ -41,6 +42,10 @@ struct map_event {
>  	char *folder;
>  	char *old_folder;
>  	char *msg_type;
> +	char *datetime;
> +	char *subject;
> +	char *sender_name;
> +	char *priority;
>  };
<snip>
> +static void parse_event_report_date_time(struct map_event *event,
> +                                                        const char *value)
> +{
> +	event->datetime = g_strdup(value);
> +}
> +
> +static void parse_event_report_subject(struct map_event *event,
> +                                                        const char *value)
> +{
> +	event->subject = g_strdup(value);
> +}
> +
> +static void parse_event_report_sender_name(struct map_event *event,
> +                                                        const char *value)
> +{
> +	event->sender_name = g_strdup(value);
> +}
> +
> +static void parse_event_report_priority(struct map_event *event,
> +                                                        const char *value)
> +{
> +	event->priority = g_strdup(value);
> +}

Where are all these freed? Don't you need to update the map_event_free()
function? If you're not yet doing so, please always test your code with
"valgrind --leak-check=full".

Johan

Re: [PATCH 1/7] obexd/client : Handle the MAP Extended Event Report 1.1

[Luiz Augusto von Dentz]

Hi,

On Thu, Sep 11, 2014 at 7:37 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Thu, Sep 11, 2014, Gowtham Anandha Babu wrote:
>>  struct map_event {
>> @@ -41,6 +42,10 @@ struct map_event {
>>       char *folder;
>>       char *old_folder;
>>       char *msg_type;
>> +     char *datetime;
>> +     char *subject;
>> +     char *sender_name;
>> +     char *priority;
>>  };
> <snip>
>> +static void parse_event_report_date_time(struct map_event *event,
>> +                                                        const char *value)
>> +{
>> +     event->datetime = g_strdup(value);
>> +}
>> +
>> +static void parse_event_report_subject(struct map_event *event,
>> +                                                        const char *value)
>> +{
>> +     event->subject = g_strdup(value);
>> +}
>> +
>> +static void parse_event_report_sender_name(struct map_event *event,
>> +                                                        const char *value)
>> +{
>> +     event->sender_name = g_strdup(value);
>> +}
>> +
>> +static void parse_event_report_priority(struct map_event *event,
>> +                                                        const char *value)
>> +{
>> +     event->priority = g_strdup(value);
>> +}
>
> Where are all these freed? Don't you need to update the map_event_free()
> function? If you're not yet doing so, please always test your code with
> "valgrind --leak-check=full".

And it is mentioned in the HACKING document.

-- 
Luiz Augusto von Dentz

Re: [PATCH 2/7] obexd/client/map : Handle MAP ReadStatusChanged event type

[Luiz Augusto von Dentz]

Hi,

On Thu, Sep 11, 2014 at 4:20 PM, Gowtham Anandha Babu
<gowtham.ab@samsung.com> wrote:
> Adds ReadStatusChanged MCE event type handling in map_handle_notification()
> ---
>  obexd/client/map.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/obexd/client/map.c b/obexd/client/map.c
> index 520e492..a505707 100644
> --- a/obexd/client/map.c
> +++ b/obexd/client/map.c
> @@ -1872,6 +1872,25 @@ static void map_handle_status_changed(struct map_data *map,
>                                                                 "Status");
>  }
>
> +static void map_handle_read_status_changed(struct map_data *map,
> +                                                        struct map_event *event)
> +{
> +       struct map_msg *msg;
> +
> +       msg = g_hash_table_lookup(map->messages, &event->handle);
> +       if (msg == NULL)
> +               return;
> +
> +       /* In MAP V 1.2 specification : ReadStatusChanged event didn't have  clear explaination.
> +       So as of now it will set the message read status as "yes" if it was "no" already
> +       and the other way round. This implementation may change once we get 'read' attribute in the event report. */

The coding style for multi-line comment is wrong, please check our
coding style there is a specific chapter for it, and to be honest I
did not get it why we are toggling the value if it is not clearly
explained, btw there is a typo there, perhaps you should check against
the test specification if there is any test regarding that or any
errata documenting the behavior.

> +       if(msg->flags & MAP_MSG_FLAG_READ)
> +               parse_read(msg,"no");
> +       else
> +               parse_read(msg,"yes");
> +}
> +
>  static void map_handle_folder_changed(struct map_data *map,
>                                                         struct map_event *event,
>                                                         const char *folder)
> @@ -1927,6 +1946,9 @@ static void map_handle_notification(struct map_event *event, void *user_data)
>         case MAP_ET_MESSAGE_SHIFT:
>                 map_handle_folder_changed(map, event, event->folder);
>                 break;
> +       case MAP_ET_READ_STATUS_CHANGED:
> +               map_handle_read_status_changed(map, event);
> +               break;
>         default:
>                 break;
>         }
> --
> 1.9.1
>
> --
> 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



-- 
Luiz Augusto von Dentz

RE: [PATCH 2/7] obexd/client/map : Handle MAP ReadStatusChanged event type

[Gowtham Anandha Babu]

Hi,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, September 12, 2014 2:30 AM
> To: Gowtham Anandha Babu
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; Bharat Panda;
> cpgs@samsung.com
> Subject: Re: [PATCH 2/7] obexd/client/map : Handle MAP
> ReadStatusChanged event type
> 
> Hi,
> 
> On Thu, Sep 11, 2014 at 4:20 PM, Gowtham Anandha Babu
> <gowtham.ab@samsung.com> wrote:
> > Adds ReadStatusChanged MCE event type handling in
> > map_handle_notification()
> > ---
> >  obexd/client/map.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/obexd/client/map.c b/obexd/client/map.c index
> > 520e492..a505707 100644
> > --- a/obexd/client/map.c
> > +++ b/obexd/client/map.c
> > @@ -1872,6 +1872,25 @@ static void map_handle_status_changed(struct
> map_data *map,
> >
> > "Status");  }
> >
> > +static void map_handle_read_status_changed(struct map_data *map,
> > +                                                        struct
> > +map_event *event) {
> > +       struct map_msg *msg;
> > +
> > +       msg = g_hash_table_lookup(map->messages, &event->handle);
> > +       if (msg == NULL)
> > +               return;
> > +
> > +       /* In MAP V 1.2 specification : ReadStatusChanged event didn't have
> clear explaination.
> > +       So as of now it will set the message read status as "yes" if it was "no"
> already
> > +       and the other way round. This implementation may change once
> > + we get 'read' attribute in the event report. */
> 
> The coding style for multi-line comment is wrong, please check our coding
> style there is a specific chapter for it, and to be honest I did not get it why we
> are toggling the value if it is not clearly explained, btw there is a typo there,
> perhaps you should check against the test specification if there is any test
> regarding that or any errata documenting the behavior.
> 
> > +       if(msg->flags & MAP_MSG_FLAG_READ)
> > +               parse_read(msg,"no");
> > +       else
> > +               parse_read(msg,"yes"); }
> > +
> >  static void map_handle_folder_changed(struct map_data *map,
> >                                                         struct map_event *event,
> >                                                         const char
> > *folder) @@ -1927,6 +1946,9 @@ static void
> map_handle_notification(struct map_event *event, void *user_data)
> >         case MAP_ET_MESSAGE_SHIFT:
> >                 map_handle_folder_changed(map, event, event->folder);
> >                 break;
> > +       case MAP_ET_READ_STATUS_CHANGED:
> > +               map_handle_read_status_changed(map, event);
> > +               break;
> >         default:
> >                 break;
> >         }
> > --
> > 1.9.1
> >
> > --
> > 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
> 
> 
> 
> --
> Luiz Augusto von Dentz

I will send the updated patch with proper multi-line comment following the coding style and with modification(if any).

Btw,
As per the specification: Pg.no : 35 on MAP 1.2 spec
"ReadStatusChanged: indicates that the 'read' status of a message (see 3.1.6) has
been changed on the MSE. " 

But the actual Event Report which was provided as test case in PTS tool was:

<MAP-event-report version = "1.1">
<event type = "ReadStatusChanged" handle = "12345678" folder =
"TELECOM/MSG/INBOX" msg_type = "SMS_CDMA" subject = "Hello" datetime =
"20110221T130510" sender_name = "Jamie" priority = "yes" />
</MAP-event-report>

>From the above event report 1.1, we cannot retrieve the read status of a message. 

ReadStatusChanged event is received by the MCE only if the previous read status has been changed in the MSE device. 

There are two approaches we may follow:

1) We have to toggle the read status whenever we get the ReadstatusChanged event.
2) Or simply we can call parse_read() function without toggling as show below, because the only way read status can change is from unread to read. 

Instead of if else , we can have a single line implementation : 

parse_read(msg,"yes");

What do you think?

Regards,
Gowtham Anandha Babu

RE: [PATCH 1/7] obexd/client : Handle the MAP Extended Event Report 1.1

[Gowtham Anandha Babu]

Hi,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, September 12, 2014 2:16 AM
> To: Gowtham Anandha Babu; linux-bluetooth@vger.kernel.org; Dmitry
> Kasatkin; Bharat Panda; cpgs@samsung.com
> Subject: Re: [PATCH 1/7] obexd/client : Handle the MAP Extended Event
> Report 1.1
> 
> Hi,
> 
> On Thu, Sep 11, 2014 at 7:37 PM, Johan Hedberg
> <johan.hedberg@gmail.com> wrote:
> > Hi,
> >
> > On Thu, Sep 11, 2014, Gowtham Anandha Babu wrote:
> >>  struct map_event {
> >> @@ -41,6 +42,10 @@ struct map_event {
> >>       char *folder;
> >>       char *old_folder;
> >>       char *msg_type;
> >> +     char *datetime;
> >> +     char *subject;
> >> +     char *sender_name;
> >> +     char *priority;
> >>  };
> > <snip>
> >> +static void parse_event_report_date_time(struct map_event *event,
> >> +                                                        const char
> >> +*value) {
> >> +     event->datetime = g_strdup(value); }
> >> +
> >> +static void parse_event_report_subject(struct map_event *event,
> >> +                                                        const char
> >> +*value) {
> >> +     event->subject = g_strdup(value); }
> >> +
> >> +static void parse_event_report_sender_name(struct map_event
> *event,
> >> +                                                        const char
> >> +*value) {
> >> +     event->sender_name = g_strdup(value); }
> >> +
> >> +static void parse_event_report_priority(struct map_event *event,
> >> +                                                        const char
> >> +*value) {
> >> +     event->priority = g_strdup(value); }
> >
> > Where are all these freed? Don't you need to update the
> > map_event_free() function? If you're not yet doing so, please always
> > test your code with "valgrind --leak-check=full".
> 
> And it is mentioned in the HACKING document.
> 
> --
> Luiz Augusto von Dentz

I had sent the updated patch with  freeing all the event attributes and tested with valgrind(No leak). Please have a look at it.

Regards,

Gowtham Anandha Babu

Re: [PATCH 7/7] android/hal-utils.c : Fix null pointer dereference

[Szymon Janc]

Hi Gowtham,

On Thursday 11 of September 2014 18:50:07 Gowtham Anandha Babu wrote:

Commit header should be like:
android/hal-utils: Fix null pointer dereference

> Handles the possible null pointer dereference: bd_addr by checking it
> against null. ---

Commit body should be limited to 72 characters. 

>  android/hal-utils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/android/hal-utils.c b/android/hal-utils.c
> index ceefefc..64ab5a1 100644
> --- a/android/hal-utils.c
> +++ b/android/hal-utils.c
> @@ -166,11 +166,13 @@ int int2str_findstr(const char *str, const struct
> int2str m[]) */
>  const char *bt_bdaddr_t2str(const bt_bdaddr_t *bd_addr, char *buf)
>  {
> -	const uint8_t *p = bd_addr->address;
> +	const uint8_t *p;
> 
>  	if (!bd_addr)
>  		return strcpy(buf, "NULL");
> 
> +	p = bd_addr->address;
> +
>  	snprintf(buf, MAX_ADDR_STR_LEN, "%02x:%02x:%02x:%02x:%02x:%02x",
>  					p[0], p[1], p[2], p[3], p[4], p[5]);

I've fixed those minors myself and pushed the patch, but please keep it in 
mind for any future submissions. Thanks.


-- 
BR
Szymon Janc