Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Martin Mathieson]

On Wed, May 18, 2011 at 10:12 PM, Jeff Morriss wrote:

> Martin Mathieson wrote:
>
>
>>
>> On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki <
>> darkjames...@darkjames.pl > wrote:
>>
>>On Wed, May 18, 2011 at 05:39:32PM +0200, Alexis La Goutte wrote:
>> > I think it is better to add this check in checkAPIs.pl script
>>
>>Well it's little hard to do this in checkAPIs.pl cause we need
>>preprocessor,
>>and some value_string (like tpncp) are generated from file.
>>
>>This patch is OK for me.
>>
>>
>>
>> I didn't measure, but it didn't noticibly add to the startup time (I ran
>> ./wireshark >& duplicated.txt).  It is ugly to dump so much into a
>> console probably not many non-developers run wireshark that way though.
>>  I wasn't planning on submitting it in its current form.
>>
>
> Well, it serves a useful purpose.  It could always be checked in but #if'd
> out (only to be turned on periodically by an interested developer).
>
>
I submitted it with #if 0 for now.  It would be good to be able to configure
with developer checks.  I'm thinking that seeing such warnings would annoy
me into fixing them...

This reminds me of another patch I wrote a long time ago (but that I can no
longer find).  It looked at the bitmask and worked out if there were any 0s
between the first and last 1s.  I know that I've added some fields with gaps
in them myself, but genuine cases must be pretty rare  This one might
lend itself better to scripting though.

Martin


> ___
> Sent via:Wireshark-dev mailing list 
> Archives:http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Jeff Morriss]

Martin Mathieson wrote:



On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki 
mailto:darkjames...@darkjames.pl>> wrote:


On Wed, May 18, 2011 at 05:39:32PM +0200, Alexis La Goutte wrote:
 > I think it is better to add this check in checkAPIs.pl script

Well it's little hard to do this in checkAPIs.pl cause we need
preprocessor,
and some value_string (like tpncp) are generated from file.

This patch is OK for me.



I didn't measure, but it didn't noticibly add to the startup time (I ran 
./wireshark >& duplicated.txt).  It is ugly to dump so much into a 
console probably not many non-developers run wireshark that way 
though.  I wasn't planning on submitting it in its current form.


Well, it serves a useful purpose.  It could always be checked in but 
#if'd out (only to be turned on periodically by an interested developer).

___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Ed Beroset]

Jeff Morriss wrote:
>Jakub Zawadzki wrote:
>> On Wed, May 18, 2011 at 05:10:09PM +0100, Martin Mathieson wrote:
>>> On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki  wrote:
 This patch is OK for me.
>>> I didn't measure, but it didn't noticibly add to the startup time
>> 
>> This O(n^2) loop sucks a little, you can optimized it with some hashing 
>> or bit-setting/checking. 
>> But really please don't care about startup-time. It's not so important.
>
>Well, I'd disagree with startup time not being important... :-)  I 
>sometimes start Wireshark many times a day, sometimes on not-very-fast 
>SPARCs.

Speaking of more limited platforms, I wonder about about a way of reducing both 
startup time and memory usage by having the dissectors dynamically loaded (as 
with the current plug-in mechanism) rather than statically linked.  The current 
model of adding all dissectors to the main code means that Wireshark will keep 
getting bigger and bigger.  I wonder if it might not be time to ponder if 
that's the best possible option.

Ed
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Jeff Morriss]

Jakub Zawadzki wrote:

On Wed, May 18, 2011 at 05:10:09PM +0100, Martin Mathieson wrote:

On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki  wrote:

This patch is OK for me.

I didn't measure, but it didn't noticibly add to the startup time


This O(n^2) loop sucks a little, you can optimized it with some hashing 
or bit-setting/checking. 
But really please don't care about startup-time. It's not so important.


Well, I'd disagree with startup time not being important... :-)  I 
sometimes start Wireshark many times a day, sometimes on not-very-fast 
SPARCs.



probably not many non-developers run wireshark that way though.
I wasn't planning on submitting it in its current form.


I think non-developers really don't care about whole tmp_fld_check_assert() 
check,
so one extra is good (and yours patch is really cool).

tmp_fld_check_assert() probably should be #ifdef-ed in some MAINTAINER_BUILD 
(or #ifndef RELEASE_BUILD)


I'd thought of that before, but we definitely want it enabled for all 
non-release builds (some of those checks save you a crash later on). 
Even the first compiles in the release builds should have it enabled to 
ensure ft sanity.


And, these days, these checks are also useful for scripters (Lua or 
Python); see bug 5930 for an example of where the Lua error checking 
hadn't kept up with the tmp_fld_check_assert() checking.

___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Jakub Zawadzki]

On Wed, May 18, 2011 at 05:10:09PM +0100, Martin Mathieson wrote:
> On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki  wrote:
> > This patch is OK for me.
> 
> I didn't measure, but it didn't noticibly add to the startup time

This O(n^2) loop sucks a little, you can optimized it with some hashing 
or bit-setting/checking. 
But really please don't care about startup-time. It's not so important.

> probably not many non-developers run wireshark that way though.
> I wasn't planning on submitting it in its current form.

I think non-developers really don't care about whole tmp_fld_check_assert() 
check,
so one extra is good (and yours patch is really cool).

tmp_fld_check_assert() probably should be #ifdef-ed in some MAINTAINER_BUILD 
(or #ifndef RELEASE_BUILD)
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Martin Mathieson]

On Wed, May 18, 2011 at 4:49 PM, Jakub Zawadzki
wrote:

> On Wed, May 18, 2011 at 05:39:32PM +0200, Alexis La Goutte wrote:
> > I think it is better to add this check in checkAPIs.pl script
>
> Well it's little hard to do this in checkAPIs.pl cause we need
> preprocessor,
> and some value_string (like tpncp) are generated from file.
>
> This patch is OK for me.
>


I didn't measure, but it didn't noticibly add to the startup time (I ran
./wireshark >& duplicated.txt).  It is ugly to dump so much into a
console probably not many non-developers run wireshark that way though.
I wasn't planning on submitting it in its current form.

Martin



> ___
> Sent via:Wireshark-dev mailing list 
> Archives:http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Jakub Zawadzki]

On Wed, May 18, 2011 at 05:39:32PM +0200, Alexis La Goutte wrote:
> I think it is better to add this check in checkAPIs.pl script

Well it's little hard to do this in checkAPIs.pl cause we need preprocessor,
and some value_string (like tpncp) are generated from file.

This patch is OK for me.
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe

Re: [Wireshark-dev] Finding duplicate (conflicting) value_string entries

[Alexis La Goutte]

On Wed, May 18, 2011 at 1:13 PM, Martin Mathieson <
martin.r.mathie...@googlemail.com> wrote:

> Hi,
>
> I've written a patch to check for duplicate entries (i.e. the same value)
> in the value_string used for hf items.
> What I found was there are places where error codes are taken from multiple
> places, but often have identical strings, so there is no real harm...
>
> The patch I'm attaching also checks in the case of matching values that the
> string is the same too, and warns on those.  I also attached the output I
> just got from starting up wireshark here.  I don't think this patch should
> be checked in, but it might possible be to do a similar check in one of the
> scripts in the tools folder?
>
> Best regards,
> Martin


Hi,
Thanks !
I think it is better to add this check in checkAPIs.pl script

Also I'm fix issue in packet-isakmp.c file (about cp_version table) in
rev37240

Regards
___
Sent via:Wireshark-dev mailing list 
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe