Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 8579] Dissector for ASTERIX packets

[bugzilla-daemon@xxxxxxxxxxxxx]

Comment # 3 on bug 8579 from Marko Hrastovec

(In reply to comment #2)

Hi Evan, thanks for the review and comments. Please find my answers in the text
below.

> Hi Marko, thanks for the patch. Some notes from an initial review:
> 
> - The indentation is inconsistent which makes it difficult to review. Adding
> modelines to the bottom of the file
> (https://www.wireshark.org/tools/modelines.html) may help depending on which
> editor you use.

Hopefully fixed. Editor was set to use spaces instead of tabs, however it
obviously did not change tabs on code already written with tabs. I also put
modeline comments in the source.

> - You do a great deal of proto_tree_append_text - I'm not familiar with the
> protocol, but generally most cases are handled better with properly named
> and typed fields. Wireshark's formatter does a good job already with common
> types. Since you appear to have commonly-repeated custom types (which I
> think are what FIELD_PART_* are?) then you may be better off using
> BASE_CUSTOM functions (check README.developer).

I used proto_tree_append_text because ASTERIX is not consistent. Sometimes item
020 is bit field in other category it is integer. To keep the dissector as
general as possible I decided to work on FieldPart structure and present the
field's contents with proto_tree_append_text.

This way also another generality is provided. If there is a FieldPart structure
in the declaration of the ASTERIX Category it will be shown. Otherwise only the
name of the item will be displayed. That way the program works even with some
fields left undeclared.

> - You wmem_alloc a string-buffer and do some operations on it. Why not just
> create a wmem_strbuf and use that? If there's some behaviour or API that you
> would need it to do I can look at extending it.

I looked at the README.developer. Two options are presented there for strings.
Since I don't know how wmem_strbuf_append_printf works and I need snpprintf I
used the alternative. Please give me some reference where to look for strbuf
functions.

> - I'm a bit confused by the twos_complement function - could you provide
> some sort of example in the comment about what it's supposed to take and
> return? I feel like there might be an easier way to do what you're trying to
> accomplish, and hopefully one that works on little-endian systems as well.

The comment was wrong. I am using a little endian system and twos_complement
works there. This function is needed because there are cases in ASTERIX where a
number can be presented with 6 bits for instance. If this number is signed it
will be wrong when I copy it into gint64. I need to take care that negative
numbers will be decoded correctly. If we take the same example with 6 bits
number. To put it in gint64 we need to set all more significant bits to 0 if
positive and all to 1 if the number is negative.

This function is made generally to put any two's complement signed number
decoded with less that 64 bits correctly into 64bit integer.

> - Generally prefer tvb_reported_length to tvb_length since the former
> correctly handles truncated packets in captures.

Fixed.

> - The functions proto_register_asterix and proto_reg_handoff_asterix should
> be at the bottom of the file by convention so that they can be picked up
> properly by some scripts we run in the build process.

Fixed.

> - Can you provide a better explanation of what the uap array is? It's not
> obvious to me as I don't know the protocol.

Some ASTERIX Categories don't have the fixed encoding. Based on the field's
content at the beginning of the message the format of the message is different
from that point on. UAP is an acronym for User Application Profile. The
Category 001 in the source code has two profiles (plot, track). Field 020 from
that category holds the index of the uap in the second dimension of that array.
When the field 020 is decoded the current_uap is changed so the message is
decoded correctly. The first dimension determines the category (uap[1][i] is
plot, uap[1][1] is track).

To keep the declaration as general as possible I used uap array where most of
the categories have one profile. When new category is added it is only put in
the correct line in uap array. No instruction code needs to be changed.
Category is based on the first byte of the message, so 256 categories are
possible.

---

      You are receiving this mail because:

• You are watching all bug changes.