[PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal Yadav]

Support for decoding AVRCP ListPlayerApplicationSettingValues
added in Bluetooth monitor.
---
 monitor/avctp.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index ec2adcd..35eca02 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
 	}
 }
 
+static const char *value2str(uint8_t attr, uint8_t value)
+{
+	switch (attr) {
+	case AVRCP_ATTRIBUTE_ILEGAL:
+		return "Illegal";
+	case AVRCP_ATTRIBUTE_EQUALIZER:
+		switch (value) {
+		case 0x01:
+			return "OFF";
+		case 0x02:
+			return "ON";
+		default:
+			return "Reserved";
+		}
+	case AVRCP_ATTRIBUTE_REPEAT_MODE:
+		switch (value) {
+		case 0x01:
+			return "OFF";
+		case 0x02:
+			return "Single Track Repeat";
+		case 0x03:
+			return "All Track Repeat";
+		case 0x04:
+			return "Group Repeat";
+		default:
+			return "Reserved";
+		}
+	case AVRCP_ATTRIBUTE_SHUFFLE:
+		switch (value) {
+		case 0x01:
+			return "OFF";
+		case 0x02:
+			return "All Track Suffle";
+		case 0x03:
+			return "Group Suffle";
+		default:
+			return "Reserved";
+		}
+	case AVRCP_ATTRIBUTE_SCAN:
+		switch (value) {
+		case 0x01:
+			return "OFF";
+		case 0x02:
+			return "All Track Scan";
+		case 0x03:
+			return "Group Scan";
+		default:
+			return "Reserved";
+		}
+	default:
+		return "Unknown";
+	}
+}
+
 static void avrcp_passthrough_packet(const struct l2cap_frame *frame)
 {
 }
@@ -504,6 +558,31 @@ static void avrcp_list_player_values(const struct l2cap_frame *frame,
 					uint8_t ctype, uint8_t len,
 					uint8_t indent)
 {
+	static uint8_t attr = 0;
+	uint8_t num, i;
+
+	if (len < 1) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
+		num = *((uint8_t *) frame->data);
+		print_field("%*cValueCount: 0x%02x", (indent - 8), ' ', num);
+
+		for (i = 0; num > 0; num--, i++) {
+			uint8_t value;
+
+			value = *((uint8_t *) (frame->data + 1 + i));
+			print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
+					' ', value, value2str(attr, value));
+		}
+	} else {
+		attr = *((uint8_t *) frame->data);
+		print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
+							attr, attr2str(attr));
+	}
 }
 
 static void avrcp_get_current_player_value(const struct l2cap_frame *frame,
-- 
1.9.1

[PATCH v2 2/6] monitor: Add AVRCP GetCurrentPlayerApplicationSettingValue support

[Vikrampal Yadav]

Support for decoding AVRCP GetCurrentPlayerApplicationSettingValue
added in Bluetooth monitor.
---
 monitor/avctp.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index 35eca02..4620e53 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -589,6 +589,42 @@ static void avrcp_get_current_player_value(const struct l2cap_frame *frame,
 						uint8_t ctype, uint8_t len,
 						uint8_t indent)
 {
+	uint8_t num, i;
+
+	if (len < 1) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	num = *((uint8_t *) frame->data);
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
+		print_field("%*cValueCount: 0x%02x", (indent - 8), ' ', num);
+
+		for (i = 0; num > 0; num--, i++) {
+			uint8_t attr, value;
+
+			attr = *((uint8_t *) (frame->data + 1 + (i * 2)));
+			print_field("%*cAttributeID: 0x%02x (%s)",
+				(indent - 8), ' ', attr, attr2str(attr));
+
+			value = *((uint8_t *) (frame->data + 2 + (i * 2)));
+			print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
+					' ', value, value2str(attr, value));
+		}
+	} else {
+		print_field("%*cAttributeCount: 0x%02x",
+				(indent - 8), ' ', num);
+
+		for (i = 0; num > 0; num--, i++) {
+			uint8_t attr;
+
+			attr = *((uint8_t *) (frame->data + 1 + i));
+			print_field("%*cAttributeID: 0x%02x (%s)",
+				(indent - 8), ' ', attr, attr2str(attr));
+		}
+	}
 }
 
 static void avrcp_set_player_value(const struct l2cap_frame *frame,
-- 
1.9.1

[PATCH v2 3/6] monitor: Add AVRCP SetPlayerApplicationSettingValue support

[Vikrampal Yadav]

Support for decoding AVRCP SetPlayerApplicationSettingValue
added in Bluetooth monitor.
---
 monitor/avctp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index 4620e53..9d41d0b 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -631,6 +631,31 @@ static void avrcp_set_player_value(const struct l2cap_frame *frame,
 					uint8_t ctype, uint8_t len,
 					uint8_t indent)
 {
+	uint8_t num, i;
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		return;
+
+	if (len < 1) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	num = *((uint8_t *) frame->data);
+	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
+
+	for (i = 0; num > 0; num--, i++) {
+		uint8_t attr, value;
+
+		attr = *((uint8_t *) (frame->data + 1 + (i * 2)));
+		print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
+							attr, attr2str(attr));
+
+		value = *((uint8_t *) (frame->data + 2 + (i * 2)));
+		print_field("%*cValueID: 0x%02x (%s)", (indent - 8), ' ',
+						value, value2str(attr, value));
+	}
 }
 
 static void avrcp_get_player_attribute_text(const struct l2cap_frame *frame,
-- 
1.9.1

[PATCH v2 4/6] monitor: Add AVRCP GetPlayerApplicationSettingAttributeText support

[Vikrampal Yadav]

Support for decoding AVRCP GetPlayerApplicationSettingAttributeText
added in Bluetooth monitor.
---
 monitor/avctp.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index 9d41d0b..7499c9c 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -28,6 +28,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <ctype.h>
 #include <string.h>
 #include <inttypes.h>
 
@@ -483,6 +484,39 @@ static const char *value2str(uint8_t attr, uint8_t value)
 	}
 }
 
+static const char *charset2str(uint16_t charset)
+{
+	switch (charset) {
+	case 1:
+	case 2:
+		return "Reserved";
+	case 3:
+		return "ASCII";
+	case 4:
+		return "ISO_8859-1";
+	case 5:
+		return "ISO_8859-2";
+	case 6:
+		return "ISO_8859-3";
+	case 7:
+		return "ISO_8859-4";
+	case 8:
+		return "ISO_8859-5";
+	case 9:
+		return "ISO_8859-6";
+	case 10:
+		return "ISO_8859-7";
+	case 11:
+		return "ISO_8859-8";
+	case 12:
+		return "ISO_8859-9";
+	case 106:
+		return "UTF-8";
+	default:
+		return "Unknown";
+	}
+}
+
 static void avrcp_passthrough_packet(const struct l2cap_frame *frame)
 {
 }
@@ -662,6 +696,58 @@ static void avrcp_get_player_attribute_text(const struct l2cap_frame *frame,
 						uint8_t ctype, uint8_t len,
 						uint8_t indent)
 {
+	uint8_t num, i, j;
+	uint8_t *data = (uint8_t *) frame->data;
+
+	if (len < 1) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	num = *data;
+	print_field("%*cAttributeCount: 0x%02x", (indent - 8), ' ', num);
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		goto response;
+
+	for (i = 0; num > 0; num--, i++) {
+		uint8_t attr;
+
+		attr = *(data + 1 + i);
+		print_field("%*cAttributeID: 0x%02x (%s)",
+				(indent - 8), ' ', attr, attr2str(attr));
+	}
+
+	return;
+
+response:
+	for (i = 0; num > 0; num--, i++) {
+		uint8_t attr, len;
+		uint8_t totallen = 0;
+		uint16_t charset;
+
+		attr = *(data + 1 + 4 * i + totallen);
+		print_field("%*cAttributeID: 0x%02x (%s)",
+				(indent - 8), ' ', attr, attr2str(attr));
+
+		charset = get_be16(data + 2 + 4 * i + totallen);
+		print_field("%*cCharsetID: 0x%04x (%s)",
+					(indent - 8), ' ', charset,
+					charset2str(charset));
+
+		len = *(data + 4 + 4 * i + totallen);
+		print_field("%*cStringLength: 0x%02x", (indent - 8), ' ', len);
+
+		totallen =+ len;
+
+		printf("String: ");
+		for (j = 0; len > 0; len--, j++) {
+			uint8_t c = *(data + 5 + 4 * i + totallen + j);
+			printf("%1c", isprint(c) ? c : '.');
+		}
+		printf("\n");
+	}
 }
 
 static void avrcp_get_player_value_text(const struct l2cap_frame *frame,
-- 
1.9.1

[PATCH v2 5/6] monitor: Add AVRCP GetPlayerApplicationSettingValueText support

[Vikrampal Yadav]

Support for decoding AVRCP GetPlayerApplicationSettingValueText
added in Bluetooth monitor.
---
 monitor/avctp.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index 7499c9c..e0ab981 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -754,6 +754,63 @@ static void avrcp_get_player_value_text(const struct l2cap_frame *frame,
 					uint8_t ctype, uint8_t len,
 					uint8_t indent)
 {
+	static uint8_t attr = 0;
+	uint8_t num, i, j;
+	uint8_t *data = (uint8_t *) frame->data;
+
+	if (len < 1) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		goto response;
+
+	attr = *data;
+	print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
+						attr, attr2str(attr));
+
+	num = *(data + 1);
+	print_field("%*cValueCount: 0x%02x", (indent - 8), ' ', num);
+
+	for (i = 0; num > 0; num--, i++) {
+		uint8_t value = *(data + 2 + i);
+		print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
+				' ', value, value2str(attr, value));
+	}
+
+	return;
+
+response:
+	num = *data;
+	print_field("%*cValueCount: 0x%02x", (indent - 8), ' ', num);
+
+	for (i = 0; num > 0; num--, i++) {
+		uint8_t value, len;
+		uint8_t totallen = 0;
+		uint16_t charset;
+
+		value = *(data + 1 + 4 * i + totallen);
+		print_field("%*cValueID: 0x%02x (%s)", (indent - 8), ' ',
+						value, value2str(attr, value));
+
+		charset = get_be16(data + 2 + 4 * i + totallen);
+		print_field("%*cCharsetIDID: 0x%02x (%s)", (indent - 8), ' ',
+						charset, charset2str(charset));
+
+		len = *(data + 4 + 4 * i + totallen);
+		print_field("%*cStringLength: 0x%02x", (indent - 8), ' ', len);
+
+		totallen =+ len;
+
+		printf("String: ");
+		for (j = 0; len > 0; len--, j++) {
+			uint8_t c = *(data + 5 + 4 * i + totallen + j);
+			printf("%1c", isprint(c) ? c : '.');
+		}
+		printf("\n");
+	}
 }
 
 static void avrcp_displayable_charset(const struct l2cap_frame *frame,
-- 
1.9.1

[PATCH v2 6/6] monitor: Add AVRCP InformDisplayableCharacterSet support

[Vikrampal Yadav]

Support for decoding AVRCP InformDisplayableCharacterSet added in
Bluetooth monitor.
---
 monitor/avctp.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/monitor/avctp.c b/monitor/avctp.c
index e0ab981..591fb22 100644
--- a/monitor/avctp.c
+++ b/monitor/avctp.c
@@ -817,6 +817,28 @@ static void avrcp_displayable_charset(const struct l2cap_frame *frame,
 					uint8_t ctype, uint8_t len,
 					uint8_t indent)
 {
+	uint8_t num, i;
+
+	if (ctype > AVC_CTYPE_GENERAL_INQUIRY)
+		return;
+
+	if (len < 2) {
+		print_text(COLOR_ERROR, "PDU malformed");
+		packet_hexdump(frame->data, frame->size);
+		return;
+	}
+
+	num = *((uint8_t *) frame->data);
+	print_field("%*cCharsetCount: 0x%02x", (indent - 8), ' ', num);
+
+	for (i = 0; num > 0; num--, i++) {
+		uint16_t charset;
+
+		charset = get_be16(frame->data + 1 + (2 * i));
+		print_field("%*cCharsetID: 0x%04x (%s)",
+					(indent - 8), ' ', charset,
+					charset2str(charset));
+	}
 }
 
 static void avrcp_ct_battery_status(const struct l2cap_frame *frame,
-- 
1.9.1

Re: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Luiz Augusto von Dentz]

Hi Vikrampal,

On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav <vikram.pal@samsung.com> wrote:
> Support for decoding AVRCP ListPlayerApplicationSettingValues
> added in Bluetooth monitor.
> ---
>  monitor/avctp.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/monitor/avctp.c b/monitor/avctp.c
> index ec2adcd..35eca02 100644
> --- a/monitor/avctp.c
> +++ b/monitor/avctp.c
> @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
>         }
>  }
>
> +static const char *value2str(uint8_t attr, uint8_t value)
> +{
> +       switch (attr) {
> +       case AVRCP_ATTRIBUTE_ILEGAL:
> +               return "Illegal";
> +       case AVRCP_ATTRIBUTE_EQUALIZER:
> +               switch (value) {
> +               case 0x01:
> +                       return "OFF";
> +               case 0x02:
> +                       return "ON";
> +               default:
> +                       return "Reserved";
> +               }
> +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
> +               switch (value) {
> +               case 0x01:
> +                       return "OFF";
> +               case 0x02:
> +                       return "Single Track Repeat";
> +               case 0x03:
> +                       return "All Track Repeat";
> +               case 0x04:
> +                       return "Group Repeat";
> +               default:
> +                       return "Reserved";
> +               }
> +       case AVRCP_ATTRIBUTE_SHUFFLE:
> +               switch (value) {
> +               case 0x01:
> +                       return "OFF";
> +               case 0x02:
> +                       return "All Track Suffle";
> +               case 0x03:
> +                       return "Group Suffle";
> +               default:
> +                       return "Reserved";
> +               }
> +       case AVRCP_ATTRIBUTE_SCAN:
> +               switch (value) {
> +               case 0x01:
> +                       return "OFF";
> +               case 0x02:
> +                       return "All Track Scan";
> +               case 0x03:
> +                       return "Group Scan";
> +               default:
> +                       return "Reserved";
> +               }
> +       default:
> +               return "Unknown";
> +       }
> +}
> +
>  static void avrcp_passthrough_packet(const struct l2cap_frame *frame)
>  {
>  }
> @@ -504,6 +558,31 @@ static void avrcp_list_player_values(const struct l2cap_frame *frame,
>                                         uint8_t ctype, uint8_t len,
>                                         uint8_t indent)
>  {
> +       static uint8_t attr = 0;
> +       uint8_t num, i;
> +
> +       if (len < 1) {
> +               print_text(COLOR_ERROR, "PDU malformed");
> +               packet_hexdump(frame->data, frame->size);
> +               return;
> +       }
> +
> +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
> +               num = *((uint8_t *) frame->data);
> +               print_field("%*cValueCount: 0x%02x", (indent - 8), ' ', num);
> +
> +               for (i = 0; num > 0; num--, i++) {
> +                       uint8_t value;
> +
> +                       value = *((uint8_t *) (frame->data + 1 + i));
> +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
> +                                       ' ', value, value2str(attr, value));
> +               }
> +       } else {
> +               attr = *((uint8_t *) frame->data);
> +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
> +                                                       attr, attr2str(attr));
> +       }
>  }

What I suggested regarding the goto is valid for all the patches, also
you seem to disregard my comment about advancing in the frame instead
of an offset for the very begging of the frame.

-- 
Luiz Augusto von Dentz

RE: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, August 22, 2014 1:18 PM
> To: Vikrampal Yadav
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> ListPlayerApplicationSettingValues support
> 
> Hi Vikrampal,
> 
> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav
> <vikram.pal@samsung.com> wrote:
> > Support for decoding AVRCP ListPlayerApplicationSettingValues
> > added in Bluetooth monitor.
> > ---
> >  monitor/avctp.c | 79
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 79 insertions(+)
> >
> > diff --git a/monitor/avctp.c b/monitor/avctp.c index ec2adcd..35eca02
> > 100644
> > --- a/monitor/avctp.c
> > +++ b/monitor/avctp.c
> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
> >         }
> >  }
> >
> > +static const char *value2str(uint8_t attr, uint8_t value) {
> > +       switch (attr) {
> > +       case AVRCP_ATTRIBUTE_ILEGAL:
> > +               return "Illegal";
> > +       case AVRCP_ATTRIBUTE_EQUALIZER:
> > +               switch (value) {
> > +               case 0x01:
> > +                       return "OFF";
> > +               case 0x02:
> > +                       return "ON";
> > +               default:
> > +                       return "Reserved";
> > +               }
> > +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
> > +               switch (value) {
> > +               case 0x01:
> > +                       return "OFF";
> > +               case 0x02:
> > +                       return "Single Track Repeat";
> > +               case 0x03:
> > +                       return "All Track Repeat";
> > +               case 0x04:
> > +                       return "Group Repeat";
> > +               default:
> > +                       return "Reserved";
> > +               }
> > +       case AVRCP_ATTRIBUTE_SHUFFLE:
> > +               switch (value) {
> > +               case 0x01:
> > +                       return "OFF";
> > +               case 0x02:
> > +                       return "All Track Suffle";
> > +               case 0x03:
> > +                       return "Group Suffle";
> > +               default:
> > +                       return "Reserved";
> > +               }
> > +       case AVRCP_ATTRIBUTE_SCAN:
> > +               switch (value) {
> > +               case 0x01:
> > +                       return "OFF";
> > +               case 0x02:
> > +                       return "All Track Scan";
> > +               case 0x03:
> > +                       return "Group Scan";
> > +               default:
> > +                       return "Reserved";
> > +               }
> > +       default:
> > +               return "Unknown";
> > +       }
> > +}
> > +
> >  static void avrcp_passthrough_packet(const struct l2cap_frame *frame)
> > {  } @@ -504,6 +558,31 @@ static void avrcp_list_player_values(const
> > struct l2cap_frame *frame,
> >                                         uint8_t ctype, uint8_t len,
> >                                         uint8_t indent)  {
> > +       static uint8_t attr = 0;
> > +       uint8_t num, i;
> > +
> > +       if (len < 1) {
> > +               print_text(COLOR_ERROR, "PDU malformed");
> > +               packet_hexdump(frame->data, frame->size);
> > +               return;
> > +       }
> > +
> > +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
> > +               num = *((uint8_t *) frame->data);
> > +               print_field("%*cValueCount: 0x%02x", (indent - 8), '
> > + ', num);
> > +
> > +               for (i = 0; num > 0; num--, i++) {
> > +                       uint8_t value;
> > +
> > +                       value = *((uint8_t *) (frame->data + 1 + i));
> > +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
> > +                                       ' ', value, value2str(attr, value));
> > +               }
> > +       } else {
> > +               attr = *((uint8_t *) frame->data);
> > +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
> > +                                                       attr, attr2str(attr));
> > +       }
> >  }
> 
> What I suggested regarding the goto is valid for all the patches, also you
> seem to disregard my comment about advancing in the frame instead of an
> offset for the very begging of the frame.
> 
> --
> Luiz Augusto von Dentz

I thought you suggested to use goto for deep indentations. So, wherever the nesting is
manageable, I chose to let it be as it is. Otherwise at some places where indentations goes
deep, I have made use of goto as suggested by you.

And for 2nd suggestion, I believe in any loop the loop index is an important
variable and we should use it wherever obvious pattern is encountered
in a loop. If understandability is a concern then I can put proper comments
explaining the intention.

Regards,
Vikram

Re: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Luiz Augusto von Dentz]

Hi Vikrampal,

On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@samsung.com> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> Sent: Friday, August 22, 2014 1:18 PM
>> To: Vikrampal Yadav
>> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
>> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
>> ListPlayerApplicationSettingValues support
>>
>> Hi Vikrampal,
>>
>> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav
>> <vikram.pal@samsung.com> wrote:
>> > Support for decoding AVRCP ListPlayerApplicationSettingValues
>> > added in Bluetooth monitor.
>> > ---
>> >  monitor/avctp.c | 79
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 79 insertions(+)
>> >
>> > diff --git a/monitor/avctp.c b/monitor/avctp.c index ec2adcd..35eca02
>> > 100644
>> > --- a/monitor/avctp.c
>> > +++ b/monitor/avctp.c
>> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
>> >         }
>> >  }
>> >
>> > +static const char *value2str(uint8_t attr, uint8_t value) {
>> > +       switch (attr) {
>> > +       case AVRCP_ATTRIBUTE_ILEGAL:
>> > +               return "Illegal";
>> > +       case AVRCP_ATTRIBUTE_EQUALIZER:
>> > +               switch (value) {
>> > +               case 0x01:
>> > +                       return "OFF";
>> > +               case 0x02:
>> > +                       return "ON";
>> > +               default:
>> > +                       return "Reserved";
>> > +               }
>> > +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
>> > +               switch (value) {
>> > +               case 0x01:
>> > +                       return "OFF";
>> > +               case 0x02:
>> > +                       return "Single Track Repeat";
>> > +               case 0x03:
>> > +                       return "All Track Repeat";
>> > +               case 0x04:
>> > +                       return "Group Repeat";
>> > +               default:
>> > +                       return "Reserved";
>> > +               }
>> > +       case AVRCP_ATTRIBUTE_SHUFFLE:
>> > +               switch (value) {
>> > +               case 0x01:
>> > +                       return "OFF";
>> > +               case 0x02:
>> > +                       return "All Track Suffle";
>> > +               case 0x03:
>> > +                       return "Group Suffle";
>> > +               default:
>> > +                       return "Reserved";
>> > +               }
>> > +       case AVRCP_ATTRIBUTE_SCAN:
>> > +               switch (value) {
>> > +               case 0x01:
>> > +                       return "OFF";
>> > +               case 0x02:
>> > +                       return "All Track Scan";
>> > +               case 0x03:
>> > +                       return "Group Scan";
>> > +               default:
>> > +                       return "Reserved";
>> > +               }
>> > +       default:
>> > +               return "Unknown";
>> > +       }
>> > +}
>> > +
>> >  static void avrcp_passthrough_packet(const struct l2cap_frame *frame)
>> > {  } @@ -504,6 +558,31 @@ static void avrcp_list_player_values(const
>> > struct l2cap_frame *frame,
>> >                                         uint8_t ctype, uint8_t len,
>> >                                         uint8_t indent)  {
>> > +       static uint8_t attr = 0;
>> > +       uint8_t num, i;
>> > +
>> > +       if (len < 1) {
>> > +               print_text(COLOR_ERROR, "PDU malformed");
>> > +               packet_hexdump(frame->data, frame->size);
>> > +               return;
>> > +       }
>> > +
>> > +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
>> > +               num = *((uint8_t *) frame->data);
>> > +               print_field("%*cValueCount: 0x%02x", (indent - 8), '
>> > + ', num);
>> > +
>> > +               for (i = 0; num > 0; num--, i++) {
>> > +                       uint8_t value;
>> > +
>> > +                       value = *((uint8_t *) (frame->data + 1 + i));
>> > +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
>> > +                                       ' ', value, value2str(attr, value));
>> > +               }
>> > +       } else {
>> > +               attr = *((uint8_t *) frame->data);
>> > +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
>> > +                                                       attr, attr2str(attr));
>> > +       }
>> >  }
>>
>> What I suggested regarding the goto is valid for all the patches, also you
>> seem to disregard my comment about advancing in the frame instead of an
>> offset for the very begging of the frame.
>>
>> --
>> Luiz Augusto von Dentz
>
> I thought you suggested to use goto for deep indentations. So, wherever the nesting is
> manageable, I chose to let it be as it is. Otherwise at some places where indentations goes
> deep, I have made use of goto as suggested by you.

The suggestion was to avoid nesting as much as possible and once you
do for one function it probably makes sense to do for all functions to
be consistent.

> And for 2nd suggestion, I believe in any loop the loop index is an important
> variable and we should use it wherever obvious pattern is encountered
> in a loop. If understandability is a concern then I can put proper comments
> explaining the intention.

Well this is not really relevant, what Im suggesting is to move ahead
in the frame using l2cap_frame_pull then the index can be used just to
limit the amount of times you gonna pull, also note that usually you
should check if there is still data remaining in the frame otherwise
this code can access invalid data if the counter is invalid.

-- 
Luiz Augusto von Dentz

RE: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, August 22, 2014 3:32 PM
> To: Vikrampal
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> ListPlayerApplicationSettingValues support
> 
> Hi Vikrampal,
> 
> On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@samsung.com>
> wrote:
> > Hi Luiz,
> >
> >> -----Original Message-----
> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> Sent: Friday, August 22, 2014 1:18 PM
> >> To: Vikrampal Yadav
> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> cpgs@samsung.com
> >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> >> ListPlayerApplicationSettingValues support
> >>
> >> Hi Vikrampal,
> >>
> >> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav
> >> <vikram.pal@samsung.com> wrote:
> >> > Support for decoding AVRCP ListPlayerApplicationSettingValues
> >> > added in Bluetooth monitor.
> >> > ---
> >> >  monitor/avctp.c | 79
> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 79 insertions(+)
> >> >
> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index
> >> > ec2adcd..35eca02
> >> > 100644
> >> > --- a/monitor/avctp.c
> >> > +++ b/monitor/avctp.c
> >> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
> >> >         }
> >> >  }
> >> >
> >> > +static const char *value2str(uint8_t attr, uint8_t value) {
> >> > +       switch (attr) {
> >> > +       case AVRCP_ATTRIBUTE_ILEGAL:
> >> > +               return "Illegal";
> >> > +       case AVRCP_ATTRIBUTE_EQUALIZER:
> >> > +               switch (value) {
> >> > +               case 0x01:
> >> > +                       return "OFF";
> >> > +               case 0x02:
> >> > +                       return "ON";
> >> > +               default:
> >> > +                       return "Reserved";
> >> > +               }
> >> > +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
> >> > +               switch (value) {
> >> > +               case 0x01:
> >> > +                       return "OFF";
> >> > +               case 0x02:
> >> > +                       return "Single Track Repeat";
> >> > +               case 0x03:
> >> > +                       return "All Track Repeat";
> >> > +               case 0x04:
> >> > +                       return "Group Repeat";
> >> > +               default:
> >> > +                       return "Reserved";
> >> > +               }
> >> > +       case AVRCP_ATTRIBUTE_SHUFFLE:
> >> > +               switch (value) {
> >> > +               case 0x01:
> >> > +                       return "OFF";
> >> > +               case 0x02:
> >> > +                       return "All Track Suffle";
> >> > +               case 0x03:
> >> > +                       return "Group Suffle";
> >> > +               default:
> >> > +                       return "Reserved";
> >> > +               }
> >> > +       case AVRCP_ATTRIBUTE_SCAN:
> >> > +               switch (value) {
> >> > +               case 0x01:
> >> > +                       return "OFF";
> >> > +               case 0x02:
> >> > +                       return "All Track Scan";
> >> > +               case 0x03:
> >> > +                       return "Group Scan";
> >> > +               default:
> >> > +                       return "Reserved";
> >> > +               }
> >> > +       default:
> >> > +               return "Unknown";
> >> > +       }
> >> > +}
> >> > +
> >> >  static void avrcp_passthrough_packet(const struct l2cap_frame
> >> > *frame) {  } @@ -504,6 +558,31 @@ static void
> >> > avrcp_list_player_values(const struct l2cap_frame *frame,
> >> >                                         uint8_t ctype, uint8_t len,
> >> >                                         uint8_t indent)  {
> >> > +       static uint8_t attr = 0;
> >> > +       uint8_t num, i;
> >> > +
> >> > +       if (len < 1) {
> >> > +               print_text(COLOR_ERROR, "PDU malformed");
> >> > +               packet_hexdump(frame->data, frame->size);
> >> > +               return;
> >> > +       }
> >> > +
> >> > +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
> >> > +               num = *((uint8_t *) frame->data);
> >> > +               print_field("%*cValueCount: 0x%02x", (indent - 8), '
> >> > + ', num);
> >> > +
> >> > +               for (i = 0; num > 0; num--, i++) {
> >> > +                       uint8_t value;
> >> > +
> >> > +                       value = *((uint8_t *) (frame->data + 1 + i));
> >> > +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
> >> > +                                       ' ', value, value2str(attr, value));
> >> > +               }
> >> > +       } else {
> >> > +               attr = *((uint8_t *) frame->data);
> >> > +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
> >> > +                                                       attr, attr2str(attr));
> >> > +       }
> >> >  }
> >>
> >> What I suggested regarding the goto is valid for all the patches,
> >> also you seem to disregard my comment about advancing in the frame
> >> instead of an offset for the very begging of the frame.
> >>
> >> --
> >> Luiz Augusto von Dentz
> >
> > I thought you suggested to use goto for deep indentations. So,
> > wherever the nesting is manageable, I chose to let it be as it is.
> > Otherwise at some places where indentations goes deep, I have made use
> of goto as suggested by you.
> 
> The suggestion was to avoid nesting as much as possible and once you do for
> one function it probably makes sense to do for all functions to be consistent.
> 
> > And for 2nd suggestion, I believe in any loop the loop index is an
> > important variable and we should use it wherever obvious pattern is
> > encountered in a loop. If understandability is a concern then I can
> > put proper comments explaining the intention.
> 
> Well this is not really relevant, what Im suggesting is to move ahead in the
> frame using l2cap_frame_pull then the index can be used just to limit the
> amount of times you gonna pull, also note that usually you should check if
> there is still data remaining in the frame otherwise this code can access
> invalid data if the counter is invalid.
> 
> --
> Luiz Augusto von Dentz

I'll do the changes accordingly and submit it again.

Regards,
Vikram

Re: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Luiz Augusto von Dentz]

Hi Vikrampal,

On Fri, Aug 22, 2014 at 1:09 PM, Vikrampal <vikram.pal@samsung.com> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> Sent: Friday, August 22, 2014 3:32 PM
>> To: Vikrampal
>> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
>> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
>> ListPlayerApplicationSettingValues support
>>
>> Hi Vikrampal,
>>
>> On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@samsung.com>
>> wrote:
>> > Hi Luiz,
>> >
>> >> -----Original Message-----
>> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> >> Sent: Friday, August 22, 2014 1:18 PM
>> >> To: Vikrampal Yadav
>> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
>> >> cpgs@samsung.com
>> >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
>> >> ListPlayerApplicationSettingValues support
>> >>
>> >> Hi Vikrampal,
>> >>
>> >> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav
>> >> <vikram.pal@samsung.com> wrote:
>> >> > Support for decoding AVRCP ListPlayerApplicationSettingValues
>> >> > added in Bluetooth monitor.
>> >> > ---
>> >> >  monitor/avctp.c | 79
>> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  1 file changed, 79 insertions(+)
>> >> >
>> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index
>> >> > ec2adcd..35eca02
>> >> > 100644
>> >> > --- a/monitor/avctp.c
>> >> > +++ b/monitor/avctp.c
>> >> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
>> >> >         }
>> >> >  }
>> >> >
>> >> > +static const char *value2str(uint8_t attr, uint8_t value) {
>> >> > +       switch (attr) {
>> >> > +       case AVRCP_ATTRIBUTE_ILEGAL:
>> >> > +               return "Illegal";
>> >> > +       case AVRCP_ATTRIBUTE_EQUALIZER:
>> >> > +               switch (value) {
>> >> > +               case 0x01:
>> >> > +                       return "OFF";
>> >> > +               case 0x02:
>> >> > +                       return "ON";
>> >> > +               default:
>> >> > +                       return "Reserved";
>> >> > +               }
>> >> > +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
>> >> > +               switch (value) {
>> >> > +               case 0x01:
>> >> > +                       return "OFF";
>> >> > +               case 0x02:
>> >> > +                       return "Single Track Repeat";
>> >> > +               case 0x03:
>> >> > +                       return "All Track Repeat";
>> >> > +               case 0x04:
>> >> > +                       return "Group Repeat";
>> >> > +               default:
>> >> > +                       return "Reserved";
>> >> > +               }
>> >> > +       case AVRCP_ATTRIBUTE_SHUFFLE:
>> >> > +               switch (value) {
>> >> > +               case 0x01:
>> >> > +                       return "OFF";
>> >> > +               case 0x02:
>> >> > +                       return "All Track Suffle";
>> >> > +               case 0x03:
>> >> > +                       return "Group Suffle";
>> >> > +               default:
>> >> > +                       return "Reserved";
>> >> > +               }
>> >> > +       case AVRCP_ATTRIBUTE_SCAN:
>> >> > +               switch (value) {
>> >> > +               case 0x01:
>> >> > +                       return "OFF";
>> >> > +               case 0x02:
>> >> > +                       return "All Track Scan";
>> >> > +               case 0x03:
>> >> > +                       return "Group Scan";
>> >> > +               default:
>> >> > +                       return "Reserved";
>> >> > +               }
>> >> > +       default:
>> >> > +               return "Unknown";
>> >> > +       }
>> >> > +}
>> >> > +
>> >> >  static void avrcp_passthrough_packet(const struct l2cap_frame
>> >> > *frame) {  } @@ -504,6 +558,31 @@ static void
>> >> > avrcp_list_player_values(const struct l2cap_frame *frame,
>> >> >                                         uint8_t ctype, uint8_t len,
>> >> >                                         uint8_t indent)  {
>> >> > +       static uint8_t attr = 0;
>> >> > +       uint8_t num, i;
>> >> > +
>> >> > +       if (len < 1) {
>> >> > +               print_text(COLOR_ERROR, "PDU malformed");
>> >> > +               packet_hexdump(frame->data, frame->size);
>> >> > +               return;
>> >> > +       }
>> >> > +
>> >> > +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
>> >> > +               num = *((uint8_t *) frame->data);
>> >> > +               print_field("%*cValueCount: 0x%02x", (indent - 8), '
>> >> > + ', num);
>> >> > +
>> >> > +               for (i = 0; num > 0; num--, i++) {
>> >> > +                       uint8_t value;
>> >> > +
>> >> > +                       value = *((uint8_t *) (frame->data + 1 + i));
>> >> > +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
>> >> > +                                       ' ', value, value2str(attr, value));
>> >> > +               }
>> >> > +       } else {
>> >> > +               attr = *((uint8_t *) frame->data);
>> >> > +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
>> >> > +                                                       attr, attr2str(attr));
>> >> > +       }
>> >> >  }
>> >>
>> >> What I suggested regarding the goto is valid for all the patches,
>> >> also you seem to disregard my comment about advancing in the frame
>> >> instead of an offset for the very begging of the frame.
>> >>
>> >> --
>> >> Luiz Augusto von Dentz
>> >
>> > I thought you suggested to use goto for deep indentations. So,
>> > wherever the nesting is manageable, I chose to let it be as it is.
>> > Otherwise at some places where indentations goes deep, I have made use
>> of goto as suggested by you.
>>
>> The suggestion was to avoid nesting as much as possible and once you do for
>> one function it probably makes sense to do for all functions to be consistent.
>>
>> > And for 2nd suggestion, I believe in any loop the loop index is an
>> > important variable and we should use it wherever obvious pattern is
>> > encountered in a loop. If understandability is a concern then I can
>> > put proper comments explaining the intention.
>>
>> Well this is not really relevant, what Im suggesting is to move ahead in the
>> frame using l2cap_frame_pull then the index can be used just to limit the
>> amount of times you gonna pull, also note that usually you should check if
>> there is still data remaining in the frame otherwise this code can access
>> invalid data if the counter is invalid.
>>
>> --
>> Luiz Augusto von Dentz
>
> I'll do the changes accordingly and submit it again.

Check out the patch Ive just sent, I believe with those function it
will make things a lot simpler.

-- 
Luiz Augusto von Dentz

RE: [PATCH v2 1/6] monitor: Add AVRCP ListPlayerApplicationSettingValues support

[Vikrampal]

Hi Luiz,

> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Friday, August 22, 2014 4:05 PM
> To: Vikrampal
> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin; cpgs@samsung.com
> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> ListPlayerApplicationSettingValues support
> 
> Hi Vikrampal,
> 
> On Fri, Aug 22, 2014 at 1:09 PM, Vikrampal <vikram.pal@samsung.com>
> wrote:
> > Hi Luiz,
> >
> >> -----Original Message-----
> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> Sent: Friday, August 22, 2014 3:32 PM
> >> To: Vikrampal
> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> cpgs@samsung.com
> >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> >> ListPlayerApplicationSettingValues support
> >>
> >> Hi Vikrampal,
> >>
> >> On Fri, Aug 22, 2014 at 11:27 AM, Vikrampal <vikram.pal@samsung.com>
> >> wrote:
> >> > Hi Luiz,
> >> >
> >> >> -----Original Message-----
> >> >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> >> >> Sent: Friday, August 22, 2014 1:18 PM
> >> >> To: Vikrampal Yadav
> >> >> Cc: linux-bluetooth@vger.kernel.org; Dmitry Kasatkin;
> >> >> cpgs@samsung.com
> >> >> Subject: Re: [PATCH v2 1/6] monitor: Add AVRCP
> >> >> ListPlayerApplicationSettingValues support
> >> >>
> >> >> Hi Vikrampal,
> >> >>
> >> >> On Thu, Aug 21, 2014 at 1:30 PM, Vikrampal Yadav
> >> >> <vikram.pal@samsung.com> wrote:
> >> >> > Support for decoding AVRCP ListPlayerApplicationSettingValues
> >> >> > added in Bluetooth monitor.
> >> >> > ---
> >> >> >  monitor/avctp.c | 79
> >> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  1 file changed, 79 insertions(+)
> >> >> >
> >> >> > diff --git a/monitor/avctp.c b/monitor/avctp.c index
> >> >> > ec2adcd..35eca02
> >> >> > 100644
> >> >> > --- a/monitor/avctp.c
> >> >> > +++ b/monitor/avctp.c
> >> >> > @@ -429,6 +429,60 @@ static const char *attr2str(uint8_t attr)
> >> >> >         }
> >> >> >  }
> >> >> >
> >> >> > +static const char *value2str(uint8_t attr, uint8_t value) {
> >> >> > +       switch (attr) {
> >> >> > +       case AVRCP_ATTRIBUTE_ILEGAL:
> >> >> > +               return "Illegal";
> >> >> > +       case AVRCP_ATTRIBUTE_EQUALIZER:
> >> >> > +               switch (value) {
> >> >> > +               case 0x01:
> >> >> > +                       return "OFF";
> >> >> > +               case 0x02:
> >> >> > +                       return "ON";
> >> >> > +               default:
> >> >> > +                       return "Reserved";
> >> >> > +               }
> >> >> > +       case AVRCP_ATTRIBUTE_REPEAT_MODE:
> >> >> > +               switch (value) {
> >> >> > +               case 0x01:
> >> >> > +                       return "OFF";
> >> >> > +               case 0x02:
> >> >> > +                       return "Single Track Repeat";
> >> >> > +               case 0x03:
> >> >> > +                       return "All Track Repeat";
> >> >> > +               case 0x04:
> >> >> > +                       return "Group Repeat";
> >> >> > +               default:
> >> >> > +                       return "Reserved";
> >> >> > +               }
> >> >> > +       case AVRCP_ATTRIBUTE_SHUFFLE:
> >> >> > +               switch (value) {
> >> >> > +               case 0x01:
> >> >> > +                       return "OFF";
> >> >> > +               case 0x02:
> >> >> > +                       return "All Track Suffle";
> >> >> > +               case 0x03:
> >> >> > +                       return "Group Suffle";
> >> >> > +               default:
> >> >> > +                       return "Reserved";
> >> >> > +               }
> >> >> > +       case AVRCP_ATTRIBUTE_SCAN:
> >> >> > +               switch (value) {
> >> >> > +               case 0x01:
> >> >> > +                       return "OFF";
> >> >> > +               case 0x02:
> >> >> > +                       return "All Track Scan";
> >> >> > +               case 0x03:
> >> >> > +                       return "Group Scan";
> >> >> > +               default:
> >> >> > +                       return "Reserved";
> >> >> > +               }
> >> >> > +       default:
> >> >> > +               return "Unknown";
> >> >> > +       }
> >> >> > +}
> >> >> > +
> >> >> >  static void avrcp_passthrough_packet(const struct l2cap_frame
> >> >> > *frame) {  } @@ -504,6 +558,31 @@ static void
> >> >> > avrcp_list_player_values(const struct l2cap_frame *frame,
> >> >> >                                         uint8_t ctype, uint8_t len,
> >> >> >                                         uint8_t indent)  {
> >> >> > +       static uint8_t attr = 0;
> >> >> > +       uint8_t num, i;
> >> >> > +
> >> >> > +       if (len < 1) {
> >> >> > +               print_text(COLOR_ERROR, "PDU malformed");
> >> >> > +               packet_hexdump(frame->data, frame->size);
> >> >> > +               return;
> >> >> > +       }
> >> >> > +
> >> >> > +       if (ctype > AVC_CTYPE_GENERAL_INQUIRY) {
> >> >> > +               num = *((uint8_t *) frame->data);
> >> >> > +               print_field("%*cValueCount: 0x%02x", (indent - 8), '
> >> >> > + ', num);
> >> >> > +
> >> >> > +               for (i = 0; num > 0; num--, i++) {
> >> >> > +                       uint8_t value;
> >> >> > +
> >> >> > +                       value = *((uint8_t *) (frame->data + 1 + i));
> >> >> > +                       print_field("%*cValueID: 0x%02x (%s)", (indent - 8),
> >> >> > +                                       ' ', value, value2str(attr, value));
> >> >> > +               }
> >> >> > +       } else {
> >> >> > +               attr = *((uint8_t *) frame->data);
> >> >> > +               print_field("%*cAttributeID: 0x%02x (%s)", (indent - 8), ' ',
> >> >> > +                                                       attr, attr2str(attr));
> >> >> > +       }
> >> >> >  }
> >> >>
> >> >> What I suggested regarding the goto is valid for all the patches,
> >> >> also you seem to disregard my comment about advancing in the frame
> >> >> instead of an offset for the very begging of the frame.
> >> >>
> >> >> --
> >> >> Luiz Augusto von Dentz
> >> >
> >> > I thought you suggested to use goto for deep indentations. So,
> >> > wherever the nesting is manageable, I chose to let it be as it is.
> >> > Otherwise at some places where indentations goes deep, I have made
> >> > use
> >> of goto as suggested by you.
> >>
> >> The suggestion was to avoid nesting as much as possible and once you
> >> do for one function it probably makes sense to do for all functions to be
> consistent.
> >>
> >> > And for 2nd suggestion, I believe in any loop the loop index is an
> >> > important variable and we should use it wherever obvious pattern is
> >> > encountered in a loop. If understandability is a concern then I can
> >> > put proper comments explaining the intention.
> >>
> >> Well this is not really relevant, what Im suggesting is to move ahead
> >> in the frame using l2cap_frame_pull then the index can be used just
> >> to limit the amount of times you gonna pull, also note that usually
> >> you should check if there is still data remaining in the frame
> >> otherwise this code can access invalid data if the counter is invalid.
> >>
> >> --
> >> Luiz Augusto von Dentz
> >
> > I'll do the changes accordingly and submit it again.
> 
> Check out the patch Ive just sent, I believe with those function it will make
> things a lot simpler.
> 
> --
> Luiz Augusto von Dentz

Yeah I saw your patch and it surely will make things simpler.
Thanks for your help!

Regards,
Vikram