tcpdump icon indicating copy to clipboard operation
tcpdump copied to clipboard

Published 20 hours ago • verified publisher icon the-tcpdump-group

ptp: Fix management packet fields

Open cappe987 opened this issue 11 months ago • 13 comments

bp was modified inside the function but the change was not reflected back outside, resulting in the fields afterwards accessing the wrong part of the packet.

cappe987 avatar Mar 11 '24 17:03 cappe987

None of the existing tests cover the bug, correct?

infrastation avatar Mar 11 '24 17:03 infrastation

None of the existing tests cover the bug, correct?

Correct. There are no management packets in any of the tests. Should I add a test for it?

cappe987 avatar Mar 11 '24 17:03 cappe987

Please do if you can.

infrastation avatar Mar 11 '24 17:03 infrastation

Added a test for it. As I mention in the commit message, the management packets contains a TLV at the end that Tcpdump does not support yet. I don't see a reason to add the whole TLVs, but at least printing the TLV type might be nice. We'll see if I get around to doing that some day.

cappe987 avatar Mar 11 '24 18:03 cappe987

Wait, I'm stupid. Don't merge yet. The bp needs to be updated for all the fields at the end as well...

cappe987 avatar Mar 11 '24 18:03 cappe987

There we go, now it should be correct. Apologies for the confusion.

cappe987 avatar Mar 11 '24 18:03 cappe987

What is the reason for these consecutive commas?

port id : 65535, , start boundary hops  3, , boundary hops  3, , flags  0x0, , reserved  0x0

fxlb avatar Aug 13 '24 13:08 fxlb

What is the reason for these consecutive commas?

port id : 65535, , start boundary hops  3, , boundary hops  3, , flags  0x0, , reserved  0x0

No reason, I'm apparently just blind :). Fixed it now!

cappe987 avatar Aug 13 '24 17:08 cappe987

Most of the output are like <name> : <value> Should we replace

start boundary hops  3, boundary hops  3, flags  0x0, reserved  0x0

By

start boundary hops : 3, boundary hops : 3, flags : 0x0, reserved : 0x0

? (This also avoids having two consecutive spaces in the output.)

fxlb avatar Aug 15 '24 11:08 fxlb

Unless there is a good reason to do otherwise, common punctuation says not to have a space before a colon.

infrastation avatar Aug 15 '24 11:08 infrastation

Oh how did I miss that too, I'm apparently very inattentive right now. Both the double commas and spaces have been there since the original implementation in 2019. Same with space before colon.

I don't know of any reason why there should be a space before colon. I'll push the changes fxlb said, but I'm open to removing that space like Infrastation said if desired.

cappe987 avatar Aug 15 '24 12:08 cappe987

Unless there is a good reason to do otherwise, common punctuation says not to have a space before a colon.

My proposal was to harmonize the outputs, but I'm not opposed to removing the spaces before the ":".

fxlb avatar Aug 15 '24 12:08 fxlb