Re: [Wireshark-dev] Proposed changes to make tcp.ack and tcp.seq relative
Hi Jasper, On Tue, May 05, 2020 at 01:24:33AM +0200, Jasper Bongertz wrote: > Hello Peter, > > > A request was filed earlier to add a new "tcp.ack_rel" field to ensure > > that color filters can be created that always work on the relative > > sequence numbers independent of the "Relative sequence numbers" option. > > Instead of adding a new field, I propose to change the existing ones. > > > My proposed change: > > > - Change the TCP sequence number-related fields to display the relative > >numbers when available. Fallback to raw numbers if they are simply > >not available (for example, when the "Analyze TCP sequence numbers" > >preference is disabled). > > To avoid cluttering the TCP tree with redundant fields: can we only show the > absolutes if the relatives are also displayed? I don't think it's useful to > show the absolutes twice. Sure! The fields will be hidden in the view, but you will still be able to use them in filter expressions. > > - Modify the "Relative sequence numbers" preference to affect the > >displayed value in the Info column only. > > Good. > > > - The raw fields will always be available through the existing > >tcp.ack_abs and tcp.seq_abs fields. Previously they were only visible > >when "Relative sequence numbers" was disabled. This field was added > >in Wireshark 3.2. > > I guess you mean "were only visible when "Relative sequence numbers" was > **enabled**? > At least that's what my Wireshark does, unless I'm not thinking straight right > now (at 1:30am, it's quite possible...) :-) You are right, my logic was reversed :P > > - Document these changes clearly in the release notes and corresponding > >user guides if needed. > > > > Are there any objections to this change? > > No, sounds like a good solution (the "document clearly" is indeed critical > here, > I guess). And I hadn't even noticed the new way of displaying > the relative sequence numbers in 3.2 yet :-) Cool, thanks for your reply, I was already hoping for your feedback! If there are no further objections I'll submit a patch for this. On a related note, to address one of the use cases that prompted for the new field, I added expert info to mark connections where the server accepted TCP Fast Open (TFO) data. Is that useful to have? Patch in question: https://code.wireshark.org/review/36994 -- Kind regards, Peter Wu https://lekensteyn.nl ___ Sent via:Wireshark-dev mailing list 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
Re: [Wireshark-dev] Trying to decode a TLS 1.3 with null cipher
Hi Ahmed, On Mon, May 04, 2020 at 03:12:50PM -0700, Ahmed Elsherbiny wrote: > First of all, thank you again for creating the patch. I did test it and was > able to successfully decode some messages. > My implementation uses WolfSSL v4.3.0. > > I hope the patch will be merged in, please let me know if there's any more > info you need from my end. At the moment the patch is unlikely going to be merged pending further information from the relevant draft authors. Please be very careful with deploying your information, WolfSSL appears to have a bug in the implementation of the draft: https://github.com/wolfSSL/wolfssl/issues/2945 Is your implementation actually going to be used in production? What are the reasons behind choosing this draft proposal for TLS 1.3 null ciphers if I may ask? -- Kind regards, Peter Wu https://lekensteyn.nl ___ Sent via:Wireshark-dev mailing list 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
Re: [Wireshark-dev] Proposed changes to make tcp.ack and tcp.seq relative
Hello Peter, > A request was filed earlier to add a new "tcp.ack_rel" field to ensure > that color filters can be created that always work on the relative > sequence numbers independent of the "Relative sequence numbers" option. > Instead of adding a new field, I propose to change the existing ones. > My proposed change: > - Change the TCP sequence number-related fields to display the relative >numbers when available. Fallback to raw numbers if they are simply >not available (for example, when the "Analyze TCP sequence numbers" >preference is disabled). To avoid cluttering the TCP tree with redundant fields: can we only show the absolutes if the relatives are also displayed? I don't think it's useful to show the absolutes twice. > - Modify the "Relative sequence numbers" preference to affect the >displayed value in the Info column only. Good. > - The raw fields will always be available through the existing >tcp.ack_abs and tcp.seq_abs fields. Previously they were only visible >when "Relative sequence numbers" was disabled. This field was added >in Wireshark 3.2. I guess you mean "were only visible when "Relative sequence numbers" was **enabled**? At least that's what my Wireshark does, unless I'm not thinking straight right now (at 1:30am, it's quite possible...) :-) > - Document these changes clearly in the release notes and corresponding >user guides if needed. > > Are there any objections to this change? No, sounds like a good solution (the "document clearly" is indeed critical here, I guess). And I hadn't even noticed the new way of displaying the relative sequence numbers in 3.2 yet :-) Cheers, Jasper ___ Sent via:Wireshark-dev mailing list 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
Re: [Wireshark-dev] Trying to decode a TLS 1.3 with null cipher
Hello Peter,
First of all, thank you again for creating the patch. I did test it and was
able to successfully decode some messages.
My implementation uses WolfSSL v4.3.0.
I hope the patch will be merged in, please let me know if there's any more
info you need from my end.
Regards,
Ahmed
On Sat, May 2, 2020 at 3:21 PM Peter Wu wrote:
> Hi Ahmed,
>
> I have posted a patch at https://code.wireshark.org/review/37034 which
> should allow you to see the plaintext. However there is a big open
> question about the draft specification. Can you share some more details
> on your implementation, in particular which TLS library do you use?
>
> Without more answers, this patch will not be merged.
>
> Kind regards,
> Peter
>
> On Sat, May 02, 2020 at 10:55:07AM -0700, Ahmed Elsherbiny wrote:
> > Wow this is great news, thank you Peter!
> >
> > Regards,
> > Ahmed
> >
> > On Sat, May 2, 2020 at 10:21 AM Peter Wu wrote:
> >
> > > Hi Ahmed,
> > >
> > > On Fri, May 01, 2020 at 02:10:01PM -0700, Ahmed Elsherbiny wrote:
> > > > Hello,
> > > >
> > > > I've written a dissector for a custom protocol. The dissector works
> well,
> > > > and now I'm trying to run the protocol over TLS 1.3.
> > > >
> > > > The cipher suite being used is TLS_SHA256_SHA256 (Code: 0xC0B4).
> This is
> > > a
> > > > new cipher suite, it is used for integrity and has a null cipher (The
> > > > payload is actually plaintext). It is still in draft form, here is
> the
> > > > document that describes it:
> > > >
> https://www.ietf.org/id/draft-camwinget-tls-ts13-macciphersuites-05.txt
> > > >
> > > > Looking at the ServerHello packet, Wireshark shows the CipherSuite as
> > > > Unknown (0xC0B4). Consequently, it does not provide a "Decrypted
> > > > application data" tab and does not pass the data to my dissector.
> > >
> > > The new cipher name was added in the development build via commit
> > > v3.3.0rc0-513-g3e2a837cc0 (https://code.wireshark.org/review/36052).
> It
> > > is not present in the stable build yet.
> > >
> > > > This is what the TLS debug log shows:
> > > [..]
> > > > I tried adding the cipher-suite to packet-tls-utils.c and recompiling
> > > > Wireshark. This is the line that I added, since the document says
> that
> > > > Diffie-Helman is the only key exchange that can be used. I'm not
> > > completely
> > > > sure that I'm using the correct macros - I don't fully understand
> TLS.
> > > >
> > > > {0xC0B4, KEX_DH_ANON, ENC_NULL, DIG_SHA256, MODE_GCM }
> > >
> > > This is not correct, TLS 1.3 has a different key exchange (KEX_TLS13)
> > > and more changes are needed to ensure that existing TLS 1.3 ciphers do
> > > not break while adding support for this new cipher.
> > >
> > > I've created a test samples for the two ciphers and posted these at
> > > https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=16543
> > >
> > > I hope to have a patch available tomorrow.
> > > --
> > > Kind regards,
> > > Peter Wu
> > > https://lekensteyn.nl
> ___
> Sent via:Wireshark-dev mailing list
> 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
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
[Wireshark-dev] Proposed changes to make tcp.ack and tcp.seq relative
Hi all, A request was filed earlier to add a new "tcp.ack_rel" field to ensure that color filters can be created that always work on the relative sequence numbers independent of the "Relative sequence numbers" option. Instead of adding a new field, I propose to change the existing ones. My proposed change: - Change the TCP sequence number-related fields to display the relative numbers when available. Fallback to raw numbers if they are simply not available (for example, when the "Analyze TCP sequence numbers" preference is disabled). - Modify the "Relative sequence numbers" preference to affect the displayed value in the Info column only. - The raw fields will always be available through the existing tcp.ack_abs and tcp.seq_abs fields. Previously they were only visible when "Relative sequence numbers" was disabled. This field was added in Wireshark 3.2. - Document these changes clearly in the release notes and corresponding user guides if needed. Are there any objections to this change? -- Kind regards, Peter Wu https://lekensteyn.nl ___ Sent via:Wireshark-dev mailing list 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
Re: [Wireshark-dev] Regenerating packet-parlay.c
On 5/4/20 11:16 AM, Luke Mewburn wrote: > On 20-05-04 10:55, Jaap Keuter wrote: > | Hi Luke, > | > | > Yes, I regenerated the code using that patch to > | > tools/wireshark_gen.py, and it builds fine across a couple of > | > platforms. > | > | Yes, I could build it too, so the generated code itself was okay. My > | quest is to regenerate the _same_ source files. > > I don't think that's going to be possible to regenerate the same as the > currently committed file, because it was generated with wireshark_gen.py > whose implementation that did not guarantee reproducible output. > > I think that unless you have access to the exact system setup that > was used to generate packet-parlay.c that you won't be able reproduce > the files as-is with the existing tool anyway. > Good point. Also this drives home the point that generated code needs to be reproducible. > > | > I wrote a simple wrapper tool idl-regen (attached) to assist each > | > regen from the idl, used with something like: > | > $ ./idl-regen ../epan/dissectors/ > | > mmccs.idl:120: Warning: Identifier 'EventType' clashes with > CORBA 3 keyword 'eventtype' > | > policy_data.idl:119: Warning: Anonymous sequences for recursive > unions are deprecated. Use a forward declaration instead. > | > policy_data.idl:123: Warning: Anonymous sequences for recursive > unions are deprecated. Use a forward declaration instead. > | > > | > I get the same .c files generated from either: > | > - CentOS 7, python 2.7.5, omniidl 4.2.0 > | > - Fedora 31, python 3.7.6, omniidl 4.2.3 > | > > | > | So the code generation is identical between Python 2 and 3, that's a > | good sign. > > Yes. > > > | > Three files get changes from git master: - packet-gias.c - > | > packet-parlay.c - packet-tango.c > | > > | > The generated code for packet-gias.c and packet-tango.c differ to > | > git master because of the following fix to idl2wrs that doesn't > | > appear to have been rerun to regenerate those files: commit > | > 443df93896 Author: Yannik Enss > | > Date: 2019-05-29 14:20:42 +0200 > | > > | > idl2wrs: fix 'undeclared identifier' error > | > > | > the 'x_octetx' variables were removed a few years back, replace > | > them with get_CDR_xxx() > | > I8cf3410d8a152c834e7019f7d1d80de3798530c3 > | > > | > | Good find. I've applied the anti-patch for this and reran code > | generation. Now the same source files come out (with the exception > | below). I've also build with these files without issue, so can > | anyone tell me what this commit achieves? It references something 'a > | few years back' which doesn't help either. If nothing I'm inclined > | to apply this anti-patch. > | > | Gerald, you seem to have 'rubber stamped' this patch series about a > | year ago. I know it's much to ask, but I can't find anything on > | this, no bug report, no wireshark-dev posts, can you tell what this > | was for and why the repository code is not the same as the code > | generated? > > I actually think the change is probably fine to leave, but I'll defer > to others. > True, but than it becomes a solution searching for a problem. I'd rather understand the problem, or find there's no problem at all, so the solution can be dropped. > > | > packet-parlay.c gets that above change as well as many other > | > changes to generated code for /* User exception filters */, > | > mostly in the ordering of the declarations and the list of > | > array entries added to proto_register_giop_parlay() variable > | > static hf_register_info hf[] > | > > | > | Yes, this was the trigger for my initial question. Why is that ordering > | different than what's committed in the repo. My position is that > | generated files must always be possible to be regenerated. > > I completely agree that generated files must be able to be reliably > regenerated from the same inputs and tools without change. > > As we both know, the issue is that the existing tool did not guarantee > reproducible output, due to the use of a python dict which did not > guarantee a particular ordering before python 3.6. > > > > | > As far as I can tell, the new packet-parlay.c has the same > | > number of lines, just in different order. > | > | That was my impression too, but there are just too many to check by hand. > | > | > | > and compared the regenerated file with that, they both still have same > | > size and number of lines, and if I sort the lines in both the files are > | > identical. (Of course, that doesn't compile... :) > | > > | > | Oh, that is indeed a nice trick to see if at least the same lines are in > the > | generated code files. I'm going to use that. > > It wasn't super robust, but it was just part of a broader group of > tests I ran. > Of course, it's partial, but
Re: [Wireshark-dev] Regenerating packet-parlay.c
On 5/4/20 11:24 AM, Alexis La Goutte wrote: > Hi, > > I have already on the past try to rebuild parlay and remember i don't get the > same output... > but like Luke say, it will be coming from dict don't sort correctly > > Luke, can you push your ./idl-regen on CMakeList.txt ? > we can (need) to add a step on buildbot for rebuild (all)dissector and check > if > we have the same output... > > Cheers Hi Alexis, So that would be these, the SNMP dissectors and what am I missing? Anyway, this can peel off in a seperate change set, I'm going to focus on cleaning up the generated IDL dissectors. Thanks, Jaap PS: I think idl-regen can be somewhat generalized, and make use of what idl2wrs does? ___ Sent via:Wireshark-dev mailing list 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
Re: [Wireshark-dev] Regenerating packet-parlay.c
Hi, I have already on the past try to rebuild parlay and remember i don't get the same output... but like Luke say, it will be coming from dict don't sort correctly Luke, can you push your ./idl-regen on CMakeList.txt ? we can (need) to add a step on buildbot for rebuild (all)dissector and check if we have the same output... Cheers On Mon, May 4, 2020 at 11:18 AM Luke Mewburn wrote: > On 20-05-04 10:55, Jaap Keuter wrote: > | Hi Luke, > | > | > Yes, I regenerated the code using that patch to > | > tools/wireshark_gen.py, and it builds fine across a couple of > | > platforms. > | > | Yes, I could build it too, so the generated code itself was okay. My > | quest is to regenerate the _same_ source files. > > I don't think that's going to be possible to regenerate the same as the > currently committed file, because it was generated with wireshark_gen.py > whose implementation that did not guarantee reproducible output. > > I think that unless you have access to the exact system setup that > was used to generate packet-parlay.c that you won't be able reproduce > the files as-is with the existing tool anyway. > > > | > I wrote a simple wrapper tool idl-regen (attached) to assist each > | > regen from the idl, used with something like: > | > $ ./idl-regen ../epan/dissectors/ > | > mmccs.idl:120: Warning: Identifier 'EventType' clashes with CORBA > 3 keyword 'eventtype' > | > policy_data.idl:119: Warning: Anonymous sequences for recursive > unions are deprecated. Use a forward declaration instead. > | > policy_data.idl:123: Warning: Anonymous sequences for recursive > unions are deprecated. Use a forward declaration instead. > | > > | > I get the same .c files generated from either: > | > - CentOS 7, python 2.7.5, omniidl 4.2.0 > | > - Fedora 31, python 3.7.6, omniidl 4.2.3 > | > > | > | So the code generation is identical between Python 2 and 3, that's a > | good sign. > > Yes. > > > | > Three files get changes from git master: - packet-gias.c - > | > packet-parlay.c - packet-tango.c > | > > | > The generated code for packet-gias.c and packet-tango.c differ to > | > git master because of the following fix to idl2wrs that doesn't > | > appear to have been rerun to regenerate those files: commit > | > 443df93896 Author: Yannik Enss > | > Date: 2019-05-29 14:20:42 +0200 > | > > | > idl2wrs: fix 'undeclared identifier' error > | > > | > the 'x_octetx' variables were removed a few years back, replace > | > them with get_CDR_xxx() > | > I8cf3410d8a152c834e7019f7d1d80de3798530c3 > | > > | > | Good find. I've applied the anti-patch for this and reran code > | generation. Now the same source files come out (with the exception > | below). I've also build with these files without issue, so can > | anyone tell me what this commit achieves? It references something 'a > | few years back' which doesn't help either. If nothing I'm inclined > | to apply this anti-patch. > | > | Gerald, you seem to have 'rubber stamped' this patch series about a > | year ago. I know it's much to ask, but I can't find anything on > | this, no bug report, no wireshark-dev posts, can you tell what this > | was for and why the repository code is not the same as the code > | generated? > > I actually think the change is probably fine to leave, but I'll defer > to others. > > > | > packet-parlay.c gets that above change as well as many other > | > changes to generated code for /* User exception filters */, > | > mostly in the ordering of the declarations and the list of > | > array entries added to proto_register_giop_parlay() variable > | > static hf_register_info hf[] > | > > | > | Yes, this was the trigger for my initial question. Why is that ordering > | different than what's committed in the repo. My position is that > | generated files must always be possible to be regenerated. > > I completely agree that generated files must be able to be reliably > regenerated from the same inputs and tools without change. > > As we both know, the issue is that the existing tool did not guarantee > reproducible output, due to the use of a python dict which did not > guarantee a particular ordering before python 3.6. > > > > | > As far as I can tell, the new packet-parlay.c has the same > | > number of lines, just in different order. > | > | That was my impression too, but there are just too many to check by > hand. > | > | > | > and compared the regenerated file with that, they both still have > same > | > size and number of lines, and if I sort the lines in both the files > are > | > identical. (Of course, that doesn't compile... :) > | > > | > | Oh, that is indeed a nice trick to see if at least the same lines are > in the > | generated code files. I'm going to use that. > > It wasn't super robust, but it was just part of a broader group of > tests I ran. > > > | > I tried doing some
Re: [Wireshark-dev] Regenerating packet-parlay.c
On 20-05-04 10:55, Jaap Keuter wrote: | Hi Luke, | | > Yes, I regenerated the code using that patch to | > tools/wireshark_gen.py, and it builds fine across a couple of | > platforms. | | Yes, I could build it too, so the generated code itself was okay. My | quest is to regenerate the _same_ source files. I don't think that's going to be possible to regenerate the same as the currently committed file, because it was generated with wireshark_gen.py whose implementation that did not guarantee reproducible output. I think that unless you have access to the exact system setup that was used to generate packet-parlay.c that you won't be able reproduce the files as-is with the existing tool anyway. | > I wrote a simple wrapper tool idl-regen (attached) to assist each | > regen from the idl, used with something like: | > $ ./idl-regen ../epan/dissectors/ | > mmccs.idl:120: Warning: Identifier 'EventType' clashes with CORBA 3 keyword 'eventtype' | > policy_data.idl:119: Warning: Anonymous sequences for recursive unions are deprecated. Use a forward declaration instead. | > policy_data.idl:123: Warning: Anonymous sequences for recursive unions are deprecated. Use a forward declaration instead. | > | > I get the same .c files generated from either: | > - CentOS 7, python 2.7.5, omniidl 4.2.0 | > - Fedora 31, python 3.7.6, omniidl 4.2.3 | > | | So the code generation is identical between Python 2 and 3, that's a | good sign. Yes. | > Three files get changes from git master: - packet-gias.c - | > packet-parlay.c - packet-tango.c | > | > The generated code for packet-gias.c and packet-tango.c differ to | > git master because of the following fix to idl2wrs that doesn't | > appear to have been rerun to regenerate those files: commit | > 443df93896 Author: Yannik Enss | > Date: 2019-05-29 14:20:42 +0200 | > | > idl2wrs: fix 'undeclared identifier' error | > | > the 'x_octetx' variables were removed a few years back, replace | > them with get_CDR_xxx() | > I8cf3410d8a152c834e7019f7d1d80de3798530c3 | > | | Good find. I've applied the anti-patch for this and reran code | generation. Now the same source files come out (with the exception | below). I've also build with these files without issue, so can | anyone tell me what this commit achieves? It references something 'a | few years back' which doesn't help either. If nothing I'm inclined | to apply this anti-patch. | | Gerald, you seem to have 'rubber stamped' this patch series about a | year ago. I know it's much to ask, but I can't find anything on | this, no bug report, no wireshark-dev posts, can you tell what this | was for and why the repository code is not the same as the code | generated? I actually think the change is probably fine to leave, but I'll defer to others. | > packet-parlay.c gets that above change as well as many other | > changes to generated code for /* User exception filters */, | > mostly in the ordering of the declarations and the list of | > array entries added to proto_register_giop_parlay() variable | > static hf_register_info hf[] | > | | Yes, this was the trigger for my initial question. Why is that ordering | different than what's committed in the repo. My position is that | generated files must always be possible to be regenerated. I completely agree that generated files must be able to be reliably regenerated from the same inputs and tools without change. As we both know, the issue is that the existing tool did not guarantee reproducible output, due to the use of a python dict which did not guarantee a particular ordering before python 3.6. | > As far as I can tell, the new packet-parlay.c has the same | > number of lines, just in different order. | | That was my impression too, but there are just too many to check by hand. | | | > and compared the regenerated file with that, they both still have same | > size and number of lines, and if I sort the lines in both the files are | > identical. (Of course, that doesn't compile... :) | > | | Oh, that is indeed a nice trick to see if at least the same lines are in the | generated code files. I'm going to use that. It wasn't super robust, but it was just part of a broader group of tests I ran. | > I tried doing some comparison of the generated object files but | > they differed too much, probably because of the reordering of the | > source lines, even though the hf[] array ends up with the same | > content in different order. | > | | Okay, so your statement is that we just have to accept that the | ordering of the the currently generated source file is going to be | different from the committed source file. Do you think we still | should introduce the ordering statements? I think we should use the OrderedDict change I proposed, which means that the ordering will be in
Re: [Wireshark-dev] Regenerating packet-parlay.c
On 5/3/20 10:35 AM, Luke Mewburn wrote:
> On 20-05-01 13:46, Jaap Keuter wrote:
> | On 5/1/20 12:02 PM, Luke Mewburn wrote:
> | > On 20-05-01 07:34, Jaap Keuter wrote:
> | > |
> | > | > On 1 May 2020, at 04:13, Luke Mewburn
> | > | > wrote: However, looking at the code some more, it appears
> | > | > that generally wireshark_gen.py generates code in the order
> | > | > the operations are defined; the exception (hah!) is the user
> | > | > exceptions.
> | > | >
> | > | > If I instead add at the top import collections and change
> | > | > get_exceptionList() from ex_hash = {} # holds a hash of
> | > | > unique exceptions. to ex_hash = collections.OrderedDict()
> | > | > # holds a hash of unique exceptions.
> | > | >
> | > | > This results in consistent generated code with both python
> | > | > 2.7 (CentOS 7) and python 3.7 (Fedora 31).
> | > | >
> | > | > I've also fixed a whitespace issue in the generated code by
> | > | > indenting the break in
> | > | > template_helper_switch_msgtype_default_end, so that it
> | > | > matches the epan/dissectors code and other default
> | > | > statements.
> | > | >
> | > | >
> | > | > Here's a patch with my suggested fixes. Or would you prefer
> | > | > a commit/pull request (etc)?
> | > | >
> | > | >
> | > | > regards, Luke.
> | > | >
> | > |
> | > | Hi Luke,
> | > |
> | > | That’s great, I didn’t have the opportunity yet to dig into
> | > | this. Nice that you compared Python 2.7 and 3.7 already.
> | > | I’ll pick this up and put it in with the other fixes I've
> | > | lined up, so you won’t have to push a change. I’ll credit you
> | > | in the commit :)
> | > |
> | > | Thanks, Jaap
> |
> | He Luke,
> |
> | Are you able to recreate the parlay dissector currently in the
> | repository with this. So far I haven't succeeded.
> |
> | Thanks, Jaap
>
Hi Luke,
> Yes, I regenerated the code using that patch to tools/wireshark_gen.py,
> and it builds fine across a couple of platforms.
Yes, I could build it too, so the generated code itself was okay. My quest is to
regenerate the _same_ source files.
>
> (BTW: I needed a separate build fix on CentOS 7 for packet-dof.c
> and I've pushed a change to review for that)
>
Yes, that was an unintended consequence of an include glib.h cleanup (of which
this is also part). You and Dario are on the case, so we'll work that out.
>
> I wrote a simple wrapper tool idl-regen (attached) to assist each
> regen from the idl, used with something like:
> $ ./idl-regen ../epan/dissectors/
> mmccs.idl:120: Warning: Identifier 'EventType' clashes with CORBA 3
> keyword 'eventtype'
> policy_data.idl:119: Warning: Anonymous sequences for recursive unions
> are deprecated. Use a forward declaration instead.
> policy_data.idl:123: Warning: Anonymous sequences for recursive unions
> are deprecated. Use a forward declaration instead.
>
> I get the same .c files generated from either:
> - CentOS 7, python 2.7.5, omniidl 4.2.0
> - Fedora 31, python 3.7.6, omniidl 4.2.3
>
So the code generation is identical between Python 2 and 3, that's a good sign.
>
> Three files get changes from git master:
> - packet-gias.c
> - packet-parlay.c
> - packet-tango.c
>
> The generated code for packet-gias.c and packet-tango.c
> differ to git master because of the following fix to idl2wrs
> that doesn't appear to have been rerun to regenerate those files:
> commit 443df93896
> Author: Yannik Enss
> Date: 2019-05-29 14:20:42 +0200
>
> idl2wrs: fix 'undeclared identifier' error
>
> the 'x_octetx' variables were removed a few years back, replace them
> with get_CDR_xxx()
> I8cf3410d8a152c834e7019f7d1d80de3798530c3
>
Good find. I've applied the anti-patch for this and reran code generation. Now
the same source files come out (with the exception below). I've also build with
these files without issue, so can anyone tell me what this commit achieves? It
references something 'a few years back' which doesn't help either. If nothing
I'm inclined to apply this anti-patch.
Gerald, you seem to have 'rubber stamped' this patch series about a year ago. I
know it's much to ask, but I can't find anything on this, no bug report, no
wireshark-dev posts, can you tell what this was for and why the repository code
is not the same as the code generated?
>
> packet-parlay.c gets that above change as well as many other
> changes to generated code for /* User exception filters */,
> mostly in the ordering of the declarations and the list of
> array entries added to proto_register_giop_parlay() variable
> static hf_register_info hf[]
>
Yes, this was the trigger for my initial question. Why is that ordering
different than what's committed in the repo. My position is that generated files
must always be possible to be
