2018-06-18 16:57:39

by João Paulo Rechi Vita

[permalink] [raw]
Subject: [PATCH] monitor: Add option to disable SCO packets

It is difficult to follow anything else happening when a SCO connection
is active, as the log gets floded with SCO packets. This adds an option
to disable dumping SCO packets.
---
monitor/main.c | 9 +++++++--
monitor/packet.c | 6 ++++--
monitor/packet.h | 1 +
3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/monitor/main.c b/monitor/main.c
index 5fa87ea3f..6a8618c41 100644
--- a/monitor/main.c
+++ b/monitor/main.c
@@ -70,7 +70,8 @@ static void usage(void)
"\t-V, --vendor <compid> Set default company identifier\n"
"\t-t, --time Show time instead of time offset\n"
"\t-T, --date Show time and date information\n"
- "\t-S, --sco Dump SCO traffic\n"
+ "\t-N, --no-sco-packets Do not dump SCO packets\n"
+ "\t-S, --sco Dump SCO traffic (without -N)\n"
"\t-A, --a2dp Dump A2DP stream traffic\n"
"\t-E, --ellisys [ip] Send Ellisys HCI Injection\n"
"\t-P, --no-pager Disable pager usage\n"
@@ -89,6 +90,7 @@ static const struct option main_options[] = {
{ "vendor", required_argument, NULL, 'V' },
{ "time", no_argument, NULL, 't' },
{ "date", no_argument, NULL, 'T' },
+ { "no-sco", no_argument, NULL, 'N' },
{ "sco", no_argument, NULL, 'S' },
{ "a2dp", no_argument, NULL, 'A' },
{ "ellisys", required_argument, NULL, 'E' },
@@ -122,7 +124,7 @@ int main(int argc, char *argv[])
int opt;
struct sockaddr_un addr;

- opt = getopt_long(argc, argv, "r:w:a:s:p:i:d:B:V:tTSAEPvh",
+ opt = getopt_long(argc, argv, "r:w:a:s:p:i:d:B:V:tTNSAEPvh",
main_options, NULL);
if (opt < 0)
break;
@@ -181,6 +183,9 @@ int main(int argc, char *argv[])
filter_mask |= PACKET_FILTER_SHOW_TIME;
filter_mask |= PACKET_FILTER_SHOW_DATE;
break;
+ case 'N':
+ filter_mask |= PACKET_FILTER_NO_SCO_PACKET;
+ break;
case 'S':
filter_mask |= PACKET_FILTER_SHOW_SCO_DATA;
break;
diff --git a/monitor/packet.c b/monitor/packet.c
index 7705d2ed5..9972b3044 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -4047,10 +4047,12 @@ void packet_monitor(struct timeval *tv, struct ucred *cred,
packet_hci_acldata(tv, cred, index, true, data, size);
break;
case BTSNOOP_OPCODE_SCO_TX_PKT:
- packet_hci_scodata(tv, cred, index, false, data, size);
+ if (!(filter_mask & PACKET_FILTER_NO_SCO_PACKET))
+ packet_hci_scodata(tv, cred, index, false, data, size);
break;
case BTSNOOP_OPCODE_SCO_RX_PKT:
- packet_hci_scodata(tv, cred, index, true, data, size);
+ if (!(filter_mask & PACKET_FILTER_NO_SCO_PACKET))
+ packet_hci_scodata(tv, cred, index, true, data, size);
break;
case BTSNOOP_OPCODE_OPEN_INDEX:
if (index < MAX_INDEX)
diff --git a/monitor/packet.h b/monitor/packet.h
index 03279e114..b35708e83 100644
--- a/monitor/packet.h
+++ b/monitor/packet.h
@@ -34,6 +34,7 @@
#define PACKET_FILTER_SHOW_ACL_DATA (1 << 4)
#define PACKET_FILTER_SHOW_SCO_DATA (1 << 5)
#define PACKET_FILTER_SHOW_A2DP_STREAM (1 << 6)
+#define PACKET_FILTER_NO_SCO_PACKET (1 << 7)

bool packet_has_filter(unsigned long filter);
void packet_set_filter(unsigned long filter);
--
2.17.1



2018-06-19 14:19:07

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] monitor: Add option to disable SCO packets

Hi Luiz,

>>> I am confused. I assumed I made SCO off by default.
>>>
>>
>> Dumping the contents for SCO packets is OFF by default (enabled by
>> -S), that is, the hex dump part on the output bellow:
>>
>> SCO Data RX: Handle 257 flags 0x00 dlen 48
>>
>> #3537
>> [hci0] 16.106123
>> 23 00 2b 00 20 00 06 00 0f 00 21 00 15 00 05 00 #.+. .....!.....
>> 1d 00 13 00 ff ff 15 00 1b 00 f8 ff f1 ff 0f 00 ................
>> 0f 00 fc ff f3 ff f8 ff f8 ff fa ff 02 00 00 00 ................
>>
>> But without -S we still log the header / flags part for each packet:
>>
>> SCO Data RX: Handle 258 flags 0x00 dlen 48
>>
>> #3541
>> [hci0] 14.128754
>>
>>> Anyhow, I don’t want to see tons of independent options. Since next thing is you tell me that AVDTP media channel is also something you want to filter out. So then we better find a generic way to apply filters and also notifications about “dropped x packets due to filter” kinda hints.
>>>
>
> We might look at doing this kind of filtering using BPF instead which
> perhaps could be loaded from a file.

doing this BPF would be best.

>> My argument behind filtering out SCO is because AFAIU it is only data,
>> not signaling. On a quick look I had assumed there was no easy way to
>> separate AVDTP signaling from data, but on a second look is actually
>> quite obvious, so you are right, that would be an obvious next step
>> :-)
>>
>> Maybe we could hide the SCO and AVDTP media channels by default? In
>> this case we could print a notification when the program starts that
>> these channels are being suppressed, and / or every time the channel
>> is established. That would be reflected on the package count printed
>> with the timestamp, and I'm not sure if that would be a problem: is
>> that number reflecting how many packets have been exchanged or
>> printed?
>>
>> As much as I like the idea of a generic configurable filter, I can't
>> look into that atm.
>
> BPF already works with Bluetooth socket so perhaps we just need a way
> to load the filter from file.
>
> @Marcel: We never applied the patch below, are you okay with it:
>
> https://www.spinics.net/lists/linux-bluetooth/msg71245.html

I somehow never saw that patch. Can you re-send it to the mailing list.

Regards

Marcel


2018-06-19 08:14:15

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH] monitor: Add option to disable SCO packets

Hi Joao,

On Mon, Jun 18, 2018 at 9:57 PM, Jo=C3=A3o Paulo Rechi Vita
<[email protected]> wrote:
> Hello Marcel, thanks for the quick reply.
>
> On Mon, Jun 18, 2018 at 10:50 AM, Marcel Holtmann <[email protected]> w=
rote:
>>
>> I am confused. I assumed I made SCO off by default.
>>
>
> Dumping the contents for SCO packets is OFF by default (enabled by
> -S), that is, the hex dump part on the output bellow:
>
> SCO Data RX: Handle 257 flags 0x00 dlen 48
>
> #3537
> [hci0] 16.106123
> 23 00 2b 00 20 00 06 00 0f 00 21 00 15 00 05 00 #.+. .....!.....
> 1d 00 13 00 ff ff 15 00 1b 00 f8 ff f1 ff 0f 00 ................
> 0f 00 fc ff f3 ff f8 ff f8 ff fa ff 02 00 00 00 ................
>
> But without -S we still log the header / flags part for each packet:
>
> SCO Data RX: Handle 258 flags 0x00 dlen 48
>
> #3541
> [hci0] 14.128754
>
>> Anyhow, I don=E2=80=99t want to see tons of independent options. Since n=
ext thing is you tell me that AVDTP media channel is also something you wan=
t to filter out. So then we better find a generic way to apply filters and =
also notifications about =E2=80=9Cdropped x packets due to filter=E2=80=9D =
kinda hints.
>>

We might look at doing this kind of filtering using BPF instead which
perhaps could be loaded from a file.

> My argument behind filtering out SCO is because AFAIU it is only data,
> not signaling. On a quick look I had assumed there was no easy way to
> separate AVDTP signaling from data, but on a second look is actually
> quite obvious, so you are right, that would be an obvious next step
> :-)
>
> Maybe we could hide the SCO and AVDTP media channels by default? In
> this case we could print a notification when the program starts that
> these channels are being suppressed, and / or every time the channel
> is established. That would be reflected on the package count printed
> with the timestamp, and I'm not sure if that would be a problem: is
> that number reflecting how many packets have been exchanged or
> printed?
>
> As much as I like the idea of a generic configurable filter, I can't
> look into that atm.

BPF already works with Bluetooth socket so perhaps we just need a way
to load the filter from file.

@Marcel: We never applied the patch below, are you okay with it:

https://www.spinics.net/lists/linux-bluetooth/msg71245.html

> Regards,
>
> --
> Jo=C3=A3o Paulo Rechi Vita
> http://about.me/jprvita
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html



--=20
Luiz Augusto von Dentz

2018-06-18 18:57:14

by João Paulo Rechi Vita

[permalink] [raw]
Subject: Re: [PATCH] monitor: Add option to disable SCO packets

Hello Marcel, thanks for the quick reply.

On Mon, Jun 18, 2018 at 10:50 AM, Marcel Holtmann <[email protected]> wro=
te:
>
> I am confused. I assumed I made SCO off by default.
>

Dumping the contents for SCO packets is OFF by default (enabled by
-S), that is, the hex dump part on the output bellow:

SCO Data RX: Handle 257 flags 0x00 dlen 48

#3537
[hci0] 16.106123
23 00 2b 00 20 00 06 00 0f 00 21 00 15 00 05 00 #.+. .....!.....
1d 00 13 00 ff ff 15 00 1b 00 f8 ff f1 ff 0f 00 ................
0f 00 fc ff f3 ff f8 ff f8 ff fa ff 02 00 00 00 ................

But without -S we still log the header / flags part for each packet:

SCO Data RX: Handle 258 flags 0x00 dlen 48

#3541
[hci0] 14.128754

> Anyhow, I don=E2=80=99t want to see tons of independent options. Since ne=
xt thing is you tell me that AVDTP media channel is also something you want=
to filter out. So then we better find a generic way to apply filters and a=
lso notifications about =E2=80=9Cdropped x packets due to filter=E2=80=9D k=
inda hints.
>

My argument behind filtering out SCO is because AFAIU it is only data,
not signaling. On a quick look I had assumed there was no easy way to
separate AVDTP signaling from data, but on a second look is actually
quite obvious, so you are right, that would be an obvious next step
:-)

Maybe we could hide the SCO and AVDTP media channels by default? In
this case we could print a notification when the program starts that
these channels are being suppressed, and / or every time the channel
is established. That would be reflected on the package count printed
with the timestamp, and I'm not sure if that would be a problem: is
that number reflecting how many packets have been exchanged or
printed?

As much as I like the idea of a generic configurable filter, I can't
look into that atm.

Regards,

--
Jo=C3=A3o Paulo Rechi Vita
http://about.me/jprvita

2018-06-18 17:50:19

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] monitor: Add option to disable SCO packets

Hi Joao Paulo,

> It is difficult to follow anything else happening when a SCO connection
> is active, as the log gets floded with SCO packets. This adds an option
> to disable dumping SCO packets.
> ---
> monitor/main.c | 9 +++++++--
> monitor/packet.c | 6 ++++--
> monitor/packet.h | 1 +
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/monitor/main.c b/monitor/main.c
> index 5fa87ea3f..6a8618c41 100644
> --- a/monitor/main.c
> +++ b/monitor/main.c
> @@ -70,7 +70,8 @@ static void usage(void)
> "\t-V, --vendor <compid> Set default company identifier\n"
> "\t-t, --time Show time instead of time offset\n"
> "\t-T, --date Show time and date information\n"
> - "\t-S, --sco Dump SCO traffic\n"
> + "\t-N, --no-sco-packets Do not dump SCO packets\n"
> + "\t-S, --sco Dump SCO traffic (without -N)\n”

I am confused. I assumed I made SCO off by default.

Anyhow, I don’t want to see tons of independent options. Since next thing is you tell me that AVDTP media channel is also something you want to filter out. So then we better find a generic way to apply filters and also notifications about “dropped x packets due to filter” kinda hints.

Regards

Marcel