Wireshark · Wireshark-bugs: [Wireshark-bugs] [Bug 9047] EPL: Dissection for WriteMultipleParameter, Node Alias, SDO/ASND structu

[bugzilla-daemon@xxxxxxxxxxxxx]

Pascal Quantin changed bug 9047

What	Removed	Added
CC		pascal.quantin@gmail.com

Comment # 10 on bug 9047 from Pascal Quantin

Hi Roland,

thanks for your patch. You will find a few comments below.

Changing all the indentation from 4 spaces to tab makes the patch really huge
without any real reason (it defeats a svn blame for example for a quick check
to easily find the reason why a code line was changed). Why not keep the
existing indentation?

Moreover I would have preferred a patch removing the tree checks in existing
code separated from the patch adding new code, but that's also a matter of
taste.

Regarding this hunk:
    item = proto_tree_add_uint_format_value(epl_tree, hf_epl_asnd_svid, tvb,
offset, 1,
                        svid, "%s (%d)",
                        val_to_str_const(svid, asnd_svid_vals, "Unknown"),
svid);
Why not simply use proto_tree_add_uint?

Regarding hf_epl_asnd_sdo_cmd_data_data, why not use FT_BYTES instead of
FT_NONE and replace all the if/else if blocks by a simple 
proto_tree_item(sdo_data_tree, hf_epl_asnd_sdo_cmd_data_data, tvb, offset,
size,
                        ENC_NA);

The 'tvb_reported_length_remaining != 0' test should be replaced by
'tvb_reported_length_remaining > 0' as it can return -1

The initialization of most (all?) variables at the beginning of
dissect_epl_sdo_command_write_multiple_by_index() function seems useless.

In the following hunk:
            if ( offsetincrement <= 0 )
            {
                datalength = tvb_reported_length_remaining ( tvb, offset );
                size = tvb_reported_length_remaining ( tvb, offset ) - (
pyldoffset );
                lastentry = TRUE;
            }
How can offsetincrement be negative? (it is defined as guint32)

Best regards,
Pascal.

---

      You are receiving this mail because:

• You are watching all bug changes.