After committing the main change for this (minus the ISUP part), I did play
around some more with this check. This included:
- checking for non-inclusive overlap (i.e. some overlapping, some not)
- check to see if collectively all of the earlier entries hide a later one
I didn't find any definite bugs, but was surprised to see the use of -ve
values being used (value_min and value_max are guint32) with an FT_INT16
field.
e.g.
{ &hf_mq_tsh_ccsid , {"CCSID.....", "mq.tsh.ccsid", FT_INT16,
BASE_DEC | BASE_RANGE_STRING, RVALS(GET_VALRV(ccsid)), 0x0, "TSH CCSID",
HFILL }},
So the field is being dissected as a 16-bit signed value.
The value_string array itself is defined (using nested macros) as:
DEF_VALRB(ccsid)
/* -4*/ DEF_VALR1(MQCCSI_AS_PUBLISHED),
/* -3*/ DEF_VALR1(MQCCSI_APPL),
/* -2*/ DEF_VALR1(MQCCSI_INHERIT),
/* -1*/ DEF_VALR1(MQCCSI_EMBEDDED),
/* 0*/ DEF_VALR3(MQCCSI_UNDEFINED, MQCCSI_UNDEFINED,
"UNDEFINED/DEFAULT/Q_MGR"),
/* >=1*/ DEF_VALR3(MQCCSI_1, MQCCSI_65535, ">=1"),
DEF_VALRE;
If you cast (gint16)(-4) to a guint32 and back again, it at least gave me
the same answer, so I believe it'll work for single values. What wouldn't
work is if you had e.g.
( -2, 2, "Something" }
As the min field will be cast to something much bigger than 2, so the (min
<= val <= max) test could never pass. However, this isn't happening, or
my existing check for max being <= min would have flagged it.
Martin
On Sat, Apr 4, 2020 at 10:26 PM Martin Mathieson <
martin.r.mathie...@googlemail.com> wrote:
> In the previous message I said " I suspect having a more complicated test
> probably find many more issues." I meant to say that I doubted it would.
>
> Have uploaded https://code.wireshark.org/review/#/c/36708/ for the
> remaining issues and the checking code itself. Only spec I couldn't find
> for was ISUP - if someone who does have the spec could look it up that'd be
> great.
> Thanks,
> Martin
>
> On Sat, Apr 4, 2020 at 9:24 PM Martin Mathieson <
> martin.r.mathie...@googlemail.com> wrote:
>
>> Yes, I'm sure I've seen an example where there is a catch-all at the end
>> of each of a number of ranges, then a catch-all covering all ranges after
>> that.
>>
>> I am still only complaining if a later item is completely hidden by a
>> single, earlier one. If I understand what you said, I suppose I could
>> check whether collectively all of the earlier items hide a later one. I
>> suspect having a more complicated test probably find many more issues.
>>
>> My plan is to fix the remaining handful of issues and check it in as
>> it is, then I'll experiment with seeing if I can find any others.
>>
>> The only other automated check along these lines I tried recently was for
>> enumerated preferences (i.e. looking for duplicated IDs or strings), but it
>> sadly(?) didn't find anything...
>>
>> Martin
>>
>> On Sat, Apr 4, 2020 at 2:53 PM Jaap Keuter <jaap.keu...@xs4all.nl> wrote:
>>
>>>
>>>
>>> On 2 Apr 2020, at 23:08, Martin Mathieson via Wireshark-dev <
>>> wireshark-dev@wireshark.org> wrote:
>>>
>>> It is common to have a 'catch-all' case for parts or all of the range,
>>> which is Ok if it comes after more specific entries. I'm wondering if its
>>> worth complaining if *part* of an entry is hidden by an earlier one?
>>> Current output from master is as below. I will try to fix them up where I
>>> can access the relevant specs, but wanted to check my understanding of how
>>> they work and how fussy we should be? I will most likely update
>>> README.dissector to make sure it is clear how it is evaluated in order.
>>>
>>>
>>> Cool stuff.
>>> I can definitely see use for catch-all-in-certain-range, opposite of
>>> filling every gap with their specifics, which is maintenance heavy. This
>>> matches the val_to_string() default string used when no match is found, but
>>> then in a higher dimension. I would say let the ranges decide, if their
>>> union is the same as the catch-all then it’s okay, otherwise mark it.
>>>
>>> just my €0.02
>>> Jaap
>>>
>>>
>>> ___________________________________________________________________________
>>> Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org>
>>> Archives: https://www.wireshark.org/lists/wireshark-dev
>>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>>> mailto:wireshark-dev-requ...@wireshark.org
>>> ?subject=unsubscribe
>>
>>
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <wireshark-dev@wireshark.org>
Archives: https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
