On Tue, Dec 1, 2020 at 7:51 AM Gerald Combs <gerald@xxxxxxxxxxxxx> wrote:
>
> On 11/26/20 11:03 AM, John Thacker wrote:
> >
> > On Thu, Nov 26, 2020 at 1:19 PM Maynard, Christopher via Wireshark-dev <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>> wrote:
> >
> > Many protocols contain subtrees, such as a header with various fields that are part of the header, and it’s convenient/logical to group those fields within the header subtree. However, doing so results in a Packet Diagram that only shows the raw bytes of the subtree rather than the individual fields contained within the subtree.
> >
> > So either I’m doing something wrong, in which case I welcome any suggestions for improving the display, or there seems to be a current limitation to the way the Packet Diagram behaves with respect to subtrees. Has anyone else noticed this?
> > ...
> >
> > Is there a way to achieve this while still grouping the fields within a subtree?
> >
> >
> > Not in a subtree currently. If you look around line 600 of ui/qt/packet-diagram.cpp, you'll see that it only groups the top level fields in each protocol.
>
> That's correct. I wanted to keep the initial implementation as simple, naive, and lazy as possible.
>
> > For the same reason, bitmask fields that are grouped together not in a subtree, using proto_tree_add_bitmask_list()
> > (like packet-rtp.c#L2072 with octet1_fields), then they are displayed separately (in master, post commit
> > https://gitlab.com/wireshark/wireshark/-/commit/7654bb260d08fdb7adeafce1877fa3c38f3465ae <https://gitlab.com/wireshark/wireshark/-/commit/7654bb260d08fdb7adeafce1877fa3c38f3465ae> ), whereas
> > for bitmask fields that are added with a subtree with proto_tree_add_bitmask() only the top level header
> > item appears.
> >
> > You can see some images here: https://gitlab.com/wireshark/wireshark/-/merge_requests/959 <https://gitlab.com/wireshark/wireshark/-/merge_requests/959>
> > There you can see bitmask fields that are displayed properly because there is no subtree.
> >
> > I agree it would be a nice enhancement to travel down into the children of items that have children, though I imagine
> > you'd have to take care in some cases; e.g., dissect_e164_msisdn() from packet-e164.[ch] is used a lot in various dissectors,
> > and has a header that has the entire number, with child that only has the country code (but not a child for the non country code digits).
> > The simplest way to descend into the subtree for a E.164 number would thus only has an entry for the country code but leave the
> > other bits blank. Or you could have issues with dealing with overlaps.
>
> Would it make sense to add second-level items only if they collectively fit the offset+size of the top-level item? In this case we'd skip the second-level country code child, but we'd add it if dissect_e164_msisdn() added a non-country code sibling field.
I think so, it is worth trying.
When it expands these subtrees I think it would be nice also if it
would put an extra thick border around the entire subtree.
So it is easy to visually see what the different subtrees are.
(See smb2 as an example, it has two subtrees in every pdu, header and
command, and nothing else)
(
I tested adding a new flag for items so you you could tell wireshark
to step down into these subtrees.
It was mainly driven by SMB2 which only has two top level items and I
wanted to at least show the content of "smb2/header" instead of it
just being a 64 byte blob
but threw it away because it would be unrealistic to go through all
dissectors manually and individually flag which trees to expand
)
>
> BTW, for 1- and possibly 2-bit fields we might want to take inspiration from https://twitter.com/vivekrj/status/1269649718059118601 and rotate the label 90 degrees.
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe