2017-09-01 08:07:43

by Andrzej Kaczmarek

[permalink] [raw]
Subject: [PATCH 1/2] doc/btsnoop: Add nop opcode

This adds 'no operation' opcode for the cases where we do not want to
include any particular payload, just the header is valid.

For example, when some packets are dropped over TTY implementation can
pass this information only in some other packet and this won't happen
until there's actually something to send. With this addition it can
just send nop after some time to indicate there were packets dropped.
---
doc/btsnoop.txt | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/doc/btsnoop.txt b/doc/btsnoop.txt
index 975a53f6d..865b81554 100644
--- a/doc/btsnoop.txt
+++ b/doc/btsnoop.txt
@@ -115,6 +115,13 @@ User Logging

User logging information.

+NOP
+-----------
+
+ Code: 0xffff
+
+ No operation.
+

TTY-based protocol
==================
--
2.14.1



2017-09-01 13:28:27

by Andrzej Kaczmarek

[permalink] [raw]
Subject: Re: [PATCH 1/2] doc/btsnoop: Add nop opcode

Hi Marcel,

On Fri, Sep 1, 2017 at 2:29 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Andrzej,
>
>> This adds 'no operation' opcode for the cases where we do not want to
>> include any particular payload, just the header is valid.
>>
>> For example, when some packets are dropped over TTY implementation can
>> pass this information only in some other packet and this won't happen
>> until there's actually something to send. With this addition it can
>> just send nop after some time to indicate there were packets dropped.
>> ---
>> doc/btsnoop.txt | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/doc/btsnoop.txt b/doc/btsnoop.txt
>> index 975a53f6d..865b81554 100644
>> --- a/doc/btsnoop.txt
>> +++ b/doc/btsnoop.txt
>> @@ -115,6 +115,13 @@ User Logging
>>
>> User logging information.
>>
>> +NOP
>> +-----------
>> +
>> + Code: 0xffff
>> +
>> + No operation.
>> +
>
> Hmmm. I am not a huge fan of this. Then I see that we defined the ext_hdr that allows to deal with the dropped packets.
>
> I need to think about this a bit since there are some other opcode extensions we have to do for certain cases where we have unexpected packets. Right now I would propose to use vendor diagnostic or system note with empty payload.

Vendor diagnostic would produce a message in btmon which may be
misleading and system note with empty payload will print some garbage
(i.e. previous contents of buffer) since message is not
null-terminated.
But in this case I can just fix btmon to skip system notes with no
payload and it will be basically the same as nop - does it sound good
enough for now?

> Regards
>
> Marcel

Best regards,
Andrzej

2017-09-01 13:26:15

by Andrzej Kaczmarek

[permalink] [raw]
Subject: Re: [PATCH 1/2] doc/btsnoop: Add nop opcode

Hi Marcel,

On Fri, Sep 1, 2017 at 2:29 PM, Marcel Holtmann <[email protected]> wrote:

> Hi Andrzej,
>
> > This adds 'no operation' opcode for the cases where we do not want to
> > include any particular payload, just the header is valid.
> >
> > For example, when some packets are dropped over TTY implementation can
> > pass this information only in some other packet and this won't happen
> > until there's actually something to send. With this addition it can
> > just send nop after some time to indicate there were packets dropped.
> > ---
> > doc/btsnoop.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/doc/btsnoop.txt b/doc/btsnoop.txt
> > index 975a53f6d..865b81554 100644
> > --- a/doc/btsnoop.txt
> > +++ b/doc/btsnoop.txt
> > @@ -115,6 +115,13 @@ User Logging
> >
> > User logging information.
> >
> > +NOP
> > +-----------
> > +
> > + Code: 0xffff
> > +
> > + No operation.
> > +
>
> Hmmm. I am not a huge fan of this. Then I see that we defined the ext_hdr
> that allows to deal with the dropped packets.
>
> I need to think about this a bit since there are some other opcode
> extensions we have to do for certain cases where we have unexpected
> packets. Right now I would propose to use vendor diagnostic or system note
> with empty payload.
>

Vendor diagnostic would produce a message in btmon which may be misleading
and system note with empty payload will print some garbage (i.e. previous
contents​ of buffer) since message is not null-terminated.
But in this case I can just fix btmon to skip system notes with no payload
and it will be basically the same as nop - does it sound good enough for
now?


>
> Regards
>
> Marce
> ​l
>

Best regards,

Andrzej​

>

2017-09-01 12:29:59

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] doc/btsnoop: Add nop opcode

Hi Andrzej,

> This adds 'no operation' opcode for the cases where we do not want to
> include any particular payload, just the header is valid.
>
> For example, when some packets are dropped over TTY implementation can
> pass this information only in some other packet and this won't happen
> until there's actually something to send. With this addition it can
> just send nop after some time to indicate there were packets dropped.
> ---
> doc/btsnoop.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/doc/btsnoop.txt b/doc/btsnoop.txt
> index 975a53f6d..865b81554 100644
> --- a/doc/btsnoop.txt
> +++ b/doc/btsnoop.txt
> @@ -115,6 +115,13 @@ User Logging
>
> User logging information.
>
> +NOP
> +-----------
> +
> + Code: 0xffff
> +
> + No operation.
> +

Hmmm. I am not a huge fan of this. Then I see that we defined the ext_hdr that allows to deal with the dropped packets.

I need to think about this a bit since there are some other opcode extensions we have to do for certain cases where we have unexpected packets. Right now I would propose to use vendor diagnostic or system note with empty payload.

Regards

Marcel


2017-09-01 08:07:44

by Andrzej Kaczmarek

[permalink] [raw]
Subject: [PATCH 2/2] monitor: Add handling of nop opcode

---
monitor/packet.c | 2 ++
src/shared/btsnoop.h | 1 +
2 files changed, 3 insertions(+)

diff --git a/monitor/packet.c b/monitor/packet.c
index f214790ac..3145b075b 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -4110,6 +4110,8 @@ void packet_monitor(struct timeval *tv, struct ucred *cred,
case BTSNOOP_OPCODE_CTRL_EVENT:
packet_ctrl_event(tv, cred, index, data, size);
break;
+ case BTSNOOP_OPCODE_NOP:
+ break;
default:
sprintf(extra_str, "(code %d len %d)", opcode, size);
print_packet(tv, cred, '*', index, NULL, COLOR_ERROR,
diff --git a/src/shared/btsnoop.h b/src/shared/btsnoop.h
index 3df8998a3..0038117ad 100644
--- a/src/shared/btsnoop.h
+++ b/src/shared/btsnoop.h
@@ -53,6 +53,7 @@
#define BTSNOOP_OPCODE_CTRL_CLOSE 15
#define BTSNOOP_OPCODE_CTRL_COMMAND 16
#define BTSNOOP_OPCODE_CTRL_EVENT 17
+#define BTSNOOP_OPCODE_NOP 65535

#define BTSNOOP_MAX_PACKET_SIZE (1486 + 4)

--
2.14.1