[Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>]

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.mathieson@xxxxxxxxxxxxxx> 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.mathieson@xxxxxxxxxxxxxx> 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.keuter@xxxxxxxxx> wrote:

On 2 Apr 2020, at 23:08, Martin Mathieson via Wireshark-dev <wireshark-dev@xxxxxxxxxxxxx> 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@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