Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
So I finally got back to this and played with some of the platform-specific features as well as some of the features that have landed in trunk since 1.8 was released. I have linked two more wireframes. == Input v2 == The first wireframe [1] is a second draft of the Input tab for the Capture Options dialogue. It is similar to my first draft, with the following differences: - Replaced Add Pipe with Add... since there are several different interface types we might want to add, and put an equivalent Delete button beside it. - Minor string change for Use promiscuous mode on all interfaces checkbox - Make it a little clearer that the Help, Start Capture and Close buttons are outside the tab frame. This applies to the other two tabs of the dialogue as well, but is sufficiently trivial that I didn't redraw them. - Add the default capture filter field that is now in trunk. I reworked this slightly by putting it into its own subframe and adding some buttons. The button that was previously doubling as a label has now been put on its own as Manage Filters A Save... button was added for creating new saved filters without opening up the Manage Filters dialogue. Compile got a string change - the term BPF is jargon that we shouldn't be exposing except in special circumstances (I debated removing this button altogether, or perhaps renaming it to Show Compiled Form..., any thoughts?). == Add Interface == The second wireframe [2] is a completely new dialogue that would be opened when the user chooses Add... in the Capture Options dialogue. It has a Name and Type fields, and a Details subframe that changes depending on which type is chosen. The rest should be fairly self-explanatory. I also plan at some point to wireframe a new version of the Edit Interface Settings dialogue that is launched currently by double-clicking on an interface, but I'm not sure when I'll get around to it. I hope to integrate the special Windows-only Interface Details panel into it if I can. Thoughts? Constructive criticism is always welcome :) Cheers, Evan [1] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/1.%20Input%20v2.jpg [2] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/4.%20Add%20Interface.jpg On Sat, Dec 8, 2012 at 11:06 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 4:17 PM, Evan Huus wrote: On Sat, Dec 8, 2012 at 10:02 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:15 PM, Evan Huus wrote: On Sat, Dec 8, 2012 at 7:57 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:49 AM, Evan Huus wrote: I have drawn and scanned three mock-up sketches (linked at the bottom), one for each of the three tabs I proposed in my previous email. Apologies in advance for my inability to write legibly or draw straight lines -- I am happy to translate for people who can't decipher it :) Again, these are just what's been kicking around in my head - I'm sure there are lots of issues with them still. Constructive criticism is very welcome. One point brought up a couple of times was that increasing the number of clicks to do a specific job is not good. And at least some uses want to navigate with the keyboard. That is my main concern when choosing multiple tabs. What do others think? I agree that increasing the number of clicks isn't good if we can avoid it, but neither is overwhelming the user with too many options at once. The current dialog has (approximately) 7 major sections, one of which is quite complex (the interface list). Different UI Design guides quote different numbers here, but most of them agree you don't want to present any more than 4-6 distinct sections at a time to prevent overload. I have to admit that neither me not Irene have particular GUI knowledge. We just started what was there and tried to improve it, without changing it too much... I don't have any particular training either, I just read a lot of random design stuff on the internet :) Haven't even done that... Keyboard navigation is quite possible with tabs (assuming they're implemented correctly) and in this case may actually be faster. In the current dialogue it takes me 16 keyboard presses to get to Enable transport name resolution from the default top of the dialogue. In the tabbed dialogue I calculate it would take me only 8, which is a significant win. Agreed. A few additional notes on each sketch: == 1. Input == This tab merges the Capture section of the current Capture Options dialogue with the current Capture Interfaces dialogue. I have removed several of the columns from the interface list, as most of them were unnecessary for a summary view and made the dialogue far wider than the actual default window size (horizontal scrolling is bad). I replaced the Packets and Packets per second columns with a The set of columns currently shown is configurable. I wasn't sure which information is important
Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
On Sat, Dec 22, 2012 at 4:37 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 22, 2012, at 8:31 PM, Evan Huus wrote: So I finally got back to this and played with some of the platform-specific features as well as some of the features that have landed in trunk since 1.8 was released. I have linked two more wireframes. == Input v2 == The first wireframe [1] is a second draft of the Input tab for the Capture Options dialogue. It is similar to my first draft, with the following differences: - Replaced Add Pipe with Add... since there are several different interface types we might want to add, and put an equivalent Delete button beside it. - Minor string change for Use promiscuous mode on all interfaces checkbox - Make it a little clearer that the Help, Start Capture and Close buttons are outside the tab frame. This applies to the other two tabs of the dialogue as well, but is sufficiently trivial that I didn't redraw them. - Add the default capture filter field that is now in trunk. I reworked this slightly by putting it into its own subframe and adding some buttons. The button that was previously doubling as a label has now been put on its own as Manage Filters A Save... button was added for creating new saved filters without opening up the Manage Filters dialogue. Compile got a string change - the term BPF is jargon that we shouldn't be exposing except in special circumstances (I debated removing this button altogether, or perhaps renaming it to Show Compiled Form..., any thoughts?). == Add Interface == The second wireframe [2] is a completely new dialogue that would be opened when the user chooses Add... in the Capture Options dialogue. It has a Name and Type fields, and a Details subframe that changes depending on which type is chosen. The rest should be fairly self-explanatory. I also plan at some point to wireframe a new version of the Edit Interface Settings dialogue that is launched currently by double-clicking on an interface, but I'm not sure when I'll get around to it. I hope to integrate the special Windows-only Interface Details panel into it if I can. Thoughts? Constructive criticism is always welcome :) The table in the Input pane shows next to each other the friendly name, and addresses (and a traffic curve). Does this fit? The friendly name can be long, as the IPv6 addresses are. That is why we currently display them not next to each other, but below. I think it ought to fit, but that is something to consider. The linked screenshot [1] shows part of the current table (on trunk) at the default width. If you line up the columns with the wireframe, then: - Capture stays the same - Interface stays the same width but shows only the friendly name instead of also showing the addresses. Names long enough to be elided should still be distinguishable at the width shown. - Link-layer header becomes Addresses and becomes marginally shorter. The IPv6 address as printed below wlan0 should fit in that column with a bit of room to spare. - Prom. Mode becomes the Traffic sparkline and becomes marginally longer (taking up whatever we can shave off of Addresses). - Snaplen becomes Options at the same width. This leaves us with a tiny bit of room left (where you can just see the left edge of the Buffer column) to allocate where we need it. Assuming I've got the approximate dimensions right then this should let us make the traffic sparkline a bit wider still.. It would be nice to have the columns to be configurable, such that I can see the settings (which are important for me) without clicking on the Options button. My apologies for being unclear - the columns should of course remain configurable, I just didn't draw that. The wireframe I drew showed only what I think the defaults should be. Evan [1] https://dl.dropbox.com/u/171647/Current_Interface_Table.png Best regards Michael Cheers, Evan [1] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/1.%20Input%20v2.jpg [2] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/4.%20Add%20Interface.jpg On Sat, Dec 8, 2012 at 11:06 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 4:17 PM, Evan Huus wrote: On Sat, Dec 8, 2012 at 10:02 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:15 PM, Evan Huus wrote: On Sat, Dec 8, 2012 at 7:57 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:49 AM, Evan Huus wrote: I have drawn and scanned three mock-up sketches (linked at the bottom), one for each of the three tabs I proposed in my previous email. Apologies in advance for my inability to write legibly or draw straight lines -- I am happy to translate for people who can't decipher it :) Again, these are just what's been kicking around in my head - I'm sure there are lots of issues with them still. Constructive criticism is very welcome. One point brought up a couple
Re: [Wireshark-dev] Canaries in Wmem
On Fri, Dec 21, 2012 at 3:05 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: Evan Huus wrote: On Fri, Dec 21, 2012 at 10:41 AM, Jeff Morriss jeff.morriss...@gmail.com wrote: Evan Huus wrote: They've been on my to-do list for a while, as emem provides them. However, I've never personally used emem's canaries, and I've never actually heard of or seen anyone else using them. Are they actually useful anymore, or has Moore's law made valgrind the better tool in all situations? Well, the canaries have helped us find (and fix) a *lot* of bugs over the years. I have this vague memory of a time when most of the fuzz failures complained of canary corruption but maybe that's an exaggeration. Hopefully the lack of canary corruption these days is a sign of improvement. :-) I think they're still useful for the automated fuzz testing because we get a fuzz failure when the fuzz-bot finds a corrupted canary. Valgrind is useful to let us humans *find* the memory corruption, but unless we're at a point where the fuzz-bot can run Valgrind instead of its normal testing, I don't think we should give up the canaries. fuzz-test.sh has a -g flag that does exactly this. Is it possible to enable that flag on the fuzz-bot or would that kill performance too much? My experience with Valgrind is that it is *many* times slower. But I guess if it is more likely to detect every memory problem it might not matter. Yes it's slow, but the fuzz-bot isn't an interactive thing, so as long as it's not taking weeks to do a pass it shouldn't matter too much. But, at least in taking a quick look at that -g flag, the bot would also need a way to declare failure (so it would know when to raise a bug). I believe lines 173-178 look for valgrind failures, but it's been a while since I put them in so they may not work :) ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Canaries in Wmem
They've been on my to-do list for a while, as emem provides them. However, I've never personally used emem's canaries, and I've never actually heard of or seen anyone else using them. Are they actually useful anymore, or has Moore's law made valgrind the better tool in all situations? If we do believe they're still useful, now's the time to suggest cool new features for them etc. Would they be used more if could be enabled with an environment variable instead of a compile flag? Are the mprotected pages actually useful, or are 99% of things caught by the simpler canaries? Any other thoughts or suggestions are of course welcome. Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46608: /trunk/ /trunk/: Makefile.am configure.ac
I have been getting a link error for dumpcap since this revision that doesn't appear to be showing up in the buildbots. Output as follows: /usr/bin/ld.bfd.real: dumpcap-capture-pcap-util-unix.o: relocation R_X86_64_32 against `.rodata.str1.8' can not be used when making a shared object; recompile with -fPIC dumpcap-capture-pcap-util-unix.o: could not read symbols: Bad value This is on a bleeding-edge Ubuntu pre-release, so it's possibly a toolchain issue on my end, but I think that's unlikely. Evan On Tue, Dec 18, 2012 at 9:09 PM, morr...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=46608 User: morriss Date: 2012/12/18 06:09 PM Log: As suggested in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8076 : Enable PIE (if the compiler supports it) when compiling dumpcap. Do this regardless of whether we're configured to install dumpcap setuid-root because some users will end up running dumpcap as root regardless of how we were configured. Directory: /trunk/ ChangesPathAction +3 -2 Makefile.am Modified +24 -0 configure.acModified ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46608: /trunk/ /trunk/: Makefile.am configure.ac
That did the trick, thanks. Evan On Wed, Dec 19, 2012 at 6:00 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: I had that problem too actually: do a make clean. It appears that the dumpcap-*.o files aren't being rebuilt automatically (and they need to be so they get compiled with -fPIE so they can be linked into dumpcap). Evan Huus wrote: I have been getting a link error for dumpcap since this revision that doesn't appear to be showing up in the buildbots. Output as follows: /usr/bin/ld.bfd.real: dumpcap-capture-pcap-util-unix.o: relocation R_X86_64_32 against `.rodata.str1.8' can not be used when making a shared object; recompile with -fPIC dumpcap-capture-pcap-util-unix.o: could not read symbols: Bad value This is on a bleeding-edge Ubuntu pre-release, so it's possibly a toolchain issue on my end, but I think that's unlikely. Evan On Tue, Dec 18, 2012 at 9:09 PM, morr...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=46608 User: morriss Date: 2012/12/18 06:09 PM Log: As suggested in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8076 : Enable PIE (if the compiler supports it) when compiling dumpcap. Do this regardless of whether we're configured to install dumpcap setuid-root because some users will end up running dumpcap as root regardless of how we were configured. Directory: /trunk/ ChangesPathAction +3 -2 Makefile.am Modified +24 -0 configure.acModified ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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] Packet Loss due to Disk Contention with Running Dumpcap in a high packet rate environment
Hi John, If you don't need the entire payload of every packet (for example, if the signalling you care about is always within the first n bytes of the header of a packet), then you can use the -s option to write only the first n bytes of each packet to disk. Otherwise, you've listed all of the other things I was going to suggest. Hope this helps, Evan On Wed, Dec 12, 2012 at 2:33 PM, John Powell jrp...@gmail.com wrote: Hi Everyone, I am using DUMPCAP to capture packets in a high packet rate environment. My operating system is: CENTOS 6.3 I am experience this problem on source compiled versions: wireshark-1.6.12 and wireshark-1.8.4. In order to allow DUMPCAP to be run as a NON-ROOT user I am using the following: setcap 'CAP_NET_RAW+eip CAP_NET_ADMIN+eip' /usr/local/bin/dumpcap -v The issue is that I am experiencing packet loss to apparent disk contention when writing the packets to the disk - see attached file: packet-loss-atop.txt To help alleviate the problem I have tried the following: Disabled SELINUX Disabled AUDIT RAID 0 (striped disks) to load share the writing out of the data ARRAY /dev/md2 level=raid0 num-devices=2 devices=/dev/sda4,/dev/sdb4 Turn off journals on ext4 tune2fs -o journal_data_writeback /dev/md2 tune2fs -O ^has_journal /dev/md2 change fstab to: UUID=.. /data ext4defaults,data=writeback 0 0 Use -B option on Dumpcap to buffer the data root /usr/local/bin/dumpcap -B 16 -i 2 -f vlan and (not vrrp and not udp port 1985 and not ether host 01:00:0c:cc:cc:cc) -g -b filesize:25 -b duration:900 -w /data/eth1.cap These changes have increased the throughput but I still experience packet loss - see attached IO Graph: packet-loss-io-graph.jpg The Vendor solutions we have looked at will not decode UNISTIM signalling properly which is requirement for this tool. Any suggestions on how to better configure either the operating system or wireshark to increase packet capture throughput will be greatly appreciated. Thanks in advance for your assistance. -John ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46513: /trunk/ /trunk/doc/: dumpcap.pod tshark.pod /trunk/: dumpcap.c tshark.c
Presumably goto packet should be moved to -G so that -g is consistent across all the tools. On Tue, Dec 11, 2012 at 9:17 PM, Maynard, Chris christopher.mayn...@gtech.com wrote: -G for Wireshark? - Chris From: wireshark-commits-boun...@wireshark.org [wireshark-commits-boun...@wireshark.org] On Behalf Of morr...@wireshark.org [morr...@wireshark.org] Sent: Tuesday, December 11, 2012 9:07 PM To: wireshark-comm...@wireshark.org Subject: [Wireshark-commits] rev 46513: /trunk/ /trunk/doc/: dumpcap.pod tshark.pod /trunk/: dumpcap.c tshark.c http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=46513 User: morriss Date: 2012/12/11 06:07 PM Log: Document the -g option to dumpcap. Add that option to tshark, too, and document it. The option can't be given to Wireshark because the GUI already has a -g (goto packet). Directory: /trunk/doc/ ChangesPath Action +7 -0 dumpcap.podModified +7 -0 tshark.pod Modified Directory: /trunk/ ChangesPath Action +1 -1 dumpcap.c Modified +2 -0 tshark.c Modified -- CONFIDENTIALITY NOTICE: The information contained in this email message is intended only for use of the intended recipient. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately delete it from your system and notify the sender by replying to this email. Thank you. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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] Capture start and capture stop icons in the toolbar
On Sat, Dec 8, 2012 at 7:57 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:49 AM, Evan Huus wrote: I have drawn and scanned three mock-up sketches (linked at the bottom), one for each of the three tabs I proposed in my previous email. Apologies in advance for my inability to write legibly or draw straight lines -- I am happy to translate for people who can't decipher it :) Again, these are just what's been kicking around in my head - I'm sure there are lots of issues with them still. Constructive criticism is very welcome. One point brought up a couple of times was that increasing the number of clicks to do a specific job is not good. And at least some uses want to navigate with the keyboard. That is my main concern when choosing multiple tabs. What do others think? I agree that increasing the number of clicks isn't good if we can avoid it, but neither is overwhelming the user with too many options at once. The current dialog has (approximately) 7 major sections, one of which is quite complex (the interface list). Different UI Design guides quote different numbers here, but most of them agree you don't want to present any more than 4-6 distinct sections at a time to prevent overload. Keyboard navigation is quite possible with tabs (assuming they're implemented correctly) and in this case may actually be faster. In the current dialogue it takes me 16 keyboard presses to get to Enable transport name resolution from the default top of the dialogue. In the tabbed dialogue I calculate it would take me only 8, which is a significant win. A few additional notes on each sketch: == 1. Input == This tab merges the Capture section of the current Capture Options dialogue with the current Capture Interfaces dialogue. I have removed several of the columns from the interface list, as most of them were unnecessary for a summary view and made the dialogue far wider than the actual default window size (horizontal scrolling is bad). I replaced the Packets and Packets per second columns with a The set of columns currently shown is configurable. I wasn't sure which information is important and which not. I thought it might depend on the use case. Personally, I never use the interfaces dialog... But why do I need the traffic to select which interface I'm going to capture. Most of the time I make the choice based on the interface name or the IP address. If it depends on the use case, then configurability is a good thing, but the default should be fairly minimal. How does one configure it? I can't click-and-drag, it has no right-click context menu, and I don't see anything obvious in Wireshark's global preferences... I included the sparklines because the current Capture Interfaces dialogue includes a live packets/second count. I suspect it's useful for new users who don't know their interfaces to be able to see that the one I want to capture on is the one all my traffic is coming on, but I'm not particularly attached to it if there isn't actually a use case for it. single sparkline column, since we're using those other places in qtshark and they give a good compact overview of the traffic level. I also kept the explicit Options button column since it's more discoverable than double-clicking. That is correct. However, we didn't go for a button to save space. We could reconsider this choice. What do others think? One of the reasons I dropped some of the other columns was to make space, since I figure the discoverability of the extra configuration options was more important than displaying read-only copies of some of those values. Consider the case where the user wants to enable/disable promiscuous mode for a specific interface. Currently they can see the value but can't edit it directly (which is potentially annoying), and to edit it they have to double-click on the row (which isn't bad, but is not as discoverable as the button). In the new design it wouldn't be visible at all, but there's a nice friendly button for more options which brings up a dialogue where they can both see and edit the value they want. One of the other design principles I was working from is that one should avoid displaying read-only copies of editable fields, as the user will want to edit them and get annoyed when they can't. I also completely got rid of the Manage Interfaces dialog by spreading its functionality around a bit. Local interfaces can be hidden/unhidden via a Hide (or Unhide, depending) option in their right-click context menu (not shown in the sketch). The Show hidden interfaces checkbox allows users to unhide otherwise-hidden interfaces temporarily. Pipes can be added via the Add Pipe... button which directly opens a file-chooser dialog. Pipes can be deleted via a Delete item in their right-click context menu (not shown in the sketch). The Add pipe button doesn't scale. Where do you manage remote interfaces (if available)? We also envisioned
Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
On Sat, Dec 8, 2012 at 10:02 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:15 PM, Evan Huus wrote: On Sat, Dec 8, 2012 at 7:57 AM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 8, 2012, at 3:49 AM, Evan Huus wrote: I have drawn and scanned three mock-up sketches (linked at the bottom), one for each of the three tabs I proposed in my previous email. Apologies in advance for my inability to write legibly or draw straight lines -- I am happy to translate for people who can't decipher it :) Again, these are just what's been kicking around in my head - I'm sure there are lots of issues with them still. Constructive criticism is very welcome. One point brought up a couple of times was that increasing the number of clicks to do a specific job is not good. And at least some uses want to navigate with the keyboard. That is my main concern when choosing multiple tabs. What do others think? I agree that increasing the number of clicks isn't good if we can avoid it, but neither is overwhelming the user with too many options at once. The current dialog has (approximately) 7 major sections, one of which is quite complex (the interface list). Different UI Design guides quote different numbers here, but most of them agree you don't want to present any more than 4-6 distinct sections at a time to prevent overload. I have to admit that neither me not Irene have particular GUI knowledge. We just started what was there and tried to improve it, without changing it too much... I don't have any particular training either, I just read a lot of random design stuff on the internet :) Keyboard navigation is quite possible with tabs (assuming they're implemented correctly) and in this case may actually be faster. In the current dialogue it takes me 16 keyboard presses to get to Enable transport name resolution from the default top of the dialogue. In the tabbed dialogue I calculate it would take me only 8, which is a significant win. Agreed. A few additional notes on each sketch: == 1. Input == This tab merges the Capture section of the current Capture Options dialogue with the current Capture Interfaces dialogue. I have removed several of the columns from the interface list, as most of them were unnecessary for a summary view and made the dialogue far wider than the actual default window size (horizontal scrolling is bad). I replaced the Packets and Packets per second columns with a The set of columns currently shown is configurable. I wasn't sure which information is important and which not. I thought it might depend on the use case. Personally, I never use the interfaces dialog... But why do I need the traffic to select which interface I'm going to capture. Most of the time I make the choice based on the interface name or the IP address. If it depends on the use case, then configurability is a good thing, but the default should be fairly minimal. I think the old version was showing a lot. That is why we started with showing it and give the user the possibility to disable columns. How does one configure it? I can't click-and-drag, it has no right-click context menu, and I don't see anything obvious in Wireshark's global preferences... Right click on the column title works for me. If that doesn't work, it is a bug. Haven't tested Windows for a while... This is actually on 1.8.2 on Ubuntu - it does work in trunk on the same machine, so perhaps this is something that got added after 1.8 was released? I included the sparklines because the current Capture Interfaces dialogue includes a live packets/second count. I suspect it's useful for new users who don't know their interfaces to be able to see that the one I want to capture on is the one all my traffic is coming on, but I'm not particularly attached to it if there isn't actually a use case for it. Maybe... Our decisions were based on our needs and the feddback we got on the mailing list. So maybe there is a need for real time graphing in the capture options dialog, just no one complained about that... single sparkline column, since we're using those other places in qtshark and they give a good compact overview of the traffic level. I also kept the explicit Options button column since it's more discoverable than double-clicking. That is correct. However, we didn't go for a button to save space. We could reconsider this choice. What do others think? One of the reasons I dropped some of the other columns was to make space, since I figure the discoverability of the extra configuration options was more important than displaying read-only copies of some of those values. Consider the case where the user wants to enable/disable promiscuous mode for a specific interface. Currently they can see the value but can't edit it directly (which is potentially annoying), and to edit it they have to double-click on the row (which isn't bad
Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
On Fri, Dec 7, 2012 at 1:47 PM, Gerald Combs ger...@wireshark.org wrote: For the Qt toolbar I created start and stop capture icons based on the media player/recorder record (circle) and stop (square) conventions[1][2]. Record makes more sense to me; we are recording packets to disk after all. It also makes things easier if we ever get around to adding a playback feature. My versions are capture_start_24.png, capture_start_active_24.png, and capture_stop_24.png in the image directory. +1 A media-player-ized capture options icon could be a record button with a superimposed wrench. I'm not sure about the interface list or restart capture buttons however. +1 for the capture options icon. I would suggest that (in qtshark) the entire interface list dialog be merged into the capture options dialog - there's a large amount of information duplicated between them and they do almost the same thing already. The dialog should generally be rethought at the same time, as the current capture options dialog is already quite busy -- perhaps splitting it into tabs is the way to go? With the dialogues merged we only need one icon, which can be the record button with a superimposed wrench. For restart capture I would think we should be using a record button with superimposed refresh circular arrow people know from web browsing. Perhaps the current reload capture file icon would be sufficient there? At some point I was hoping to see if we could get Elliott Aldrich (who made the current document icon and several interface icons) to create updated versions of the main toolbar icons including the capture ones. [1] http://commons.wikimedia.org/wiki/Tango_icons#Media [2] http://commons.wikimedia.org/wiki/GNOME_Desktop_icons#Media On 12/7/12 6:38 AM, Maynard, Chris wrote: +1 There was mention of these icons some time ago, but no changes were ever made: http://www.wireshark.org/lists/wireshark-dev/201107/msg00092.html - Chris From: wireshark-dev-boun...@wireshark.org [wireshark-dev-boun...@wireshark.org] On Behalf Of Guy Harris [g...@alum.mit.edu] Sent: Friday, December 07, 2012 4:08 AM To: wireshark-dev@wireshark.org Subject: [Wireshark-dev] Capture start and capture stop icons in the toolbar On Dec 6, 2012, at 5:46 PM, ger...@wireshark.org wrote: Use a different close button in the main toolbar. It looks better but is still wrong (on OS X at least). As long as we're playing with the toolbar: I've always found the icon on the start a capture button a bit non-obvious. I guess it's supposed to be an image of a plug-in network adapter card for some parallel bus (although that's not the first thing that comes to mind when I look at it), but: 1) the sorts of machines on which a lot of people run Wireshark have built-in network adapters and 2) even a lot of the add-on adapters out there plug into serial buses (USB, PCI Express/Thunderbolt) so a conventional PCI card might be an out-of-date icon these days, and, in addition, a number of the other sniffers I've seen use the CD player start (right-pointing triangle, pick your color), stop (square, probably red or black), and, in some cases, pause (two parallel vertical lines) icons for the capture buttons. (Pause means don't receive packets, but, if you click the pause button again, continue capturing with the same options, without discarding or saving the already-captured packets.) ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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] Capture start and capture stop icons in the toolbar
On Fri, Dec 7, 2012 at 2:41 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 7, 2012, at 8:01 PM, Evan Huus wrote: On Fri, Dec 7, 2012 at 1:47 PM, Gerald Combs ger...@wireshark.org wrote: For the Qt toolbar I created start and stop capture icons based on the media player/recorder record (circle) and stop (square) conventions[1][2]. Record makes more sense to me; we are recording packets to disk after all. It also makes things easier if we ever get around to adding a playback feature. My versions are capture_start_24.png, capture_start_active_24.png, and capture_stop_24.png in the image directory. +1 A media-player-ized capture options icon could be a record button with a superimposed wrench. I'm not sure about the interface list or restart capture buttons however. +1 for the capture options icon. I would suggest that (in qtshark) the entire interface list dialog be merged into the capture options dialog - there's a large amount of information duplicated between them and they do almost the same thing already. The dialog should generally be rethought at the same time, as the current capture options dialog is already quite busy -- perhaps splitting it into tabs is the way to go? With the dialogues Hi Evan, the capture options dialog was rethought for 1.8 to support the capturing from multiple interfaces. We wanted to clean things up on the one hand side, don't change too much on the other. That was already in trunk when I first started hacking on Wireshark :) I'm not too familiar with the old (1.6?) dialog, but after a quick glance at some old documentation screenshots it looks like the 1.8 version is already a lot cleaner than it was. So if you have concrete suggestions how to improve the capture options dialog box, Irene and myself will be more than happy to discuss it. Please provide some feedback. I have a bunch of ideas floating around in my head - I will try and do some simple wireframes tonight, but for now, a quick summary: Three tabs: Input, Output, Options - Input tab contains some melding of the list of interfaces in capture options and the list of interfaces in capture interfaces. - Output tab contains everything from the Capture File(s) section in capture options, plus possibly a few more we don't expose right now. - Options tab contains the other three sections from capture options (display options, name resolution, stop capture...) Some misc other things I've been thinking about: - it would be nice if the Capture on all interfaces checkbox lived in the column title as a master checkbox (see the In Store column at [1] for an example). - it would be nice if the Prom. Mode column contained editable checkboxes, and the Capture all in promiscuous mode was a column master checkbox as well - it's not immediately clear in the Stop Capture... section whether multiple checked options will be combined with a logical AND or a logical OR - same AND vs OR issue with the various Use multiple files options - the capture interfaces dialog has a button for each interface, whereas the capture options dialog has double-clickable rows -- I'm not sure which one is better, but we should pick one (I'm leaning towards the buttons) I'm be very happy to discuss further, this is all just back-of-a-napkin ideas right now. Cheers, Evan [1] http://dhtmlx.com/docs/products/dhtmlxGrid/samples/08_filtering/03_pro_filter_num.html Best regards Michael merged we only need one icon, which can be the record button with a superimposed wrench. For restart capture I would think we should be using a record button with superimposed refresh circular arrow people know from web browsing. Perhaps the current reload capture file icon would be sufficient there? At some point I was hoping to see if we could get Elliott Aldrich (who made the current document icon and several interface icons) to create updated versions of the main toolbar icons including the capture ones. [1] http://commons.wikimedia.org/wiki/Tango_icons#Media [2] http://commons.wikimedia.org/wiki/GNOME_Desktop_icons#Media On 12/7/12 6:38 AM, Maynard, Chris wrote: +1 There was mention of these icons some time ago, but no changes were ever made: http://www.wireshark.org/lists/wireshark-dev/201107/msg00092.html - Chris From: wireshark-dev-boun...@wireshark.org [wireshark-dev-boun...@wireshark.org] On Behalf Of Guy Harris [g...@alum.mit.edu] Sent: Friday, December 07, 2012 4:08 AM To: wireshark-dev@wireshark.org Subject: [Wireshark-dev] Capture start and capture stop icons in the toolbar On Dec 6, 2012, at 5:46 PM, ger...@wireshark.org wrote: Use a different close button in the main toolbar. It looks better but is still wrong (on OS X at least). As long as we're playing with the toolbar: I've always found the icon on the start a capture button a bit non-obvious. I guess it's supposed to be an image of a plug-in network adapter
Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
On Fri, Dec 7, 2012 at 3:46 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 7, 2012, at 9:06 PM, Evan Huus wrote: On Fri, Dec 7, 2012 at 2:41 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 7, 2012, at 8:01 PM, Evan Huus wrote: On Fri, Dec 7, 2012 at 1:47 PM, Gerald Combs ger...@wireshark.org wrote: For the Qt toolbar I created start and stop capture icons based on the media player/recorder record (circle) and stop (square) conventions[1][2]. Record makes more sense to me; we are recording packets to disk after all. It also makes things easier if we ever get around to adding a playback feature. My versions are capture_start_24.png, capture_start_active_24.png, and capture_stop_24.png in the image directory. +1 A media-player-ized capture options icon could be a record button with a superimposed wrench. I'm not sure about the interface list or restart capture buttons however. +1 for the capture options icon. I would suggest that (in qtshark) the entire interface list dialog be merged into the capture options dialog - there's a large amount of information duplicated between them and they do almost the same thing already. The dialog should generally be rethought at the same time, as the current capture options dialog is already quite busy -- perhaps splitting it into tabs is the way to go? With the dialogues Hi Evan, the capture options dialog was rethought for 1.8 to support the capturing from multiple interfaces. We wanted to clean things up on the one hand side, don't change too much on the other. That was already in trunk when I first started hacking on Wireshark :) .. OK. I'm not too familiar with the old (1.6?) dialog, but after a quick glance at some old documentation screenshots it looks like the 1.8 version is already a lot cleaner than it was. So if you have concrete suggestions how to improve the capture options dialog box, Irene and myself will be more than happy to discuss it. Please provide some feedback. I have a bunch of ideas floating around in my head - I will try and do some simple wireframes tonight, but for now, a quick summary: Three tabs: Input, Output, Options - Input tab contains some melding of the list of interfaces in capture options and the list of interfaces in capture interfaces. - Output tab contains everything from the Capture File(s) section in capture options, plus possibly a few more we don't expose right now. - Options tab contains the other three sections from capture options (display options, name resolution, stop capture...) I would like to get input also from others... Our users are somewhat used to the current layout. So we should have good reasons to change it. One reason would be to have space for more options. Not sure. My primary reason for re-organizing it this way is that the interface list is already quite large (I have six interfaces listed) and if we merge in the other dialog it will just get larger - the window will end up too big with too many controls in it at once. Open to other suggestions of course, but that's the problem I was trying to solve. Some misc other things I've been thinking about: - it would be nice if the Capture on all interfaces checkbox lived in the column title as a master checkbox (see the In Store column at [1] for an example). I think we follow (to some extend) the Human Interface Guidelines for GTK. Do they have something like this? Not that I've been able to find, though they do seem to have multiple-selection checkboxes in some circumstances (see figure 6-12 in [1]). [1] http://developer.gnome.org/hig-book/3.5/controls-check-boxes.html.en - it would be nice if the Prom. Mode column contained editable checkboxes, and the Capture all in promiscuous mode was a column master checkbox as well Same question as above. - it's not immediately clear in the Stop Capture... section whether multiple checked options will be combined with a logical AND or a logical OR Not sure, we took it over. I suspect it's using OR, but I'm not sure where to look to find out. - same AND vs OR issue with the various Use multiple files options Not sure, we took it over. - the capture interfaces dialog has a button for each interface, whereas the capture options dialog has double-clickable rows -- I'm not sure which one is better, but we should pick one (I'm leaning towards the buttons) You must be using Windows... Only on Windows you have a Details button in the capture interfaces dialog. This is different form what you get when double clicking on the capture options dialog box. In the capture options dialog box we didn't use a button to save space... I see that the Details button doesn't exist on my linux version - do we not have access to that extra information on non-windows platforms? I will send some simple sketches shortly. Evan Best regards Michael I'm be very happy to discuss further, this is all just back
Re: [Wireshark-dev] Capture start and capture stop icons in the toolbar
I have drawn and scanned three mock-up sketches (linked at the bottom), one for each of the three tabs I proposed in my previous email. Apologies in advance for my inability to write legibly or draw straight lines -- I am happy to translate for people who can't decipher it :) Again, these are just what's been kicking around in my head - I'm sure there are lots of issues with them still. Constructive criticism is very welcome. A few additional notes on each sketch: == 1. Input == This tab merges the Capture section of the current Capture Options dialogue with the current Capture Interfaces dialogue. I have removed several of the columns from the interface list, as most of them were unnecessary for a summary view and made the dialogue far wider than the actual default window size (horizontal scrolling is bad). I replaced the Packets and Packets per second columns with a single sparkline column, since we're using those other places in qtshark and they give a good compact overview of the traffic level. I also kept the explicit Options button column since it's more discoverable than double-clicking. I also completely got rid of the Manage Interfaces dialog by spreading its functionality around a bit. Local interfaces can be hidden/unhidden via a Hide (or Unhide, depending) option in their right-click context menu (not shown in the sketch). The Show hidden interfaces checkbox allows users to unhide otherwise-hidden interfaces temporarily. Pipes can be added via the Add Pipe... button which directly opens a file-chooser dialog. Pipes can be deleted via a Delete item in their right-click context menu (not shown in the sketch). There are also a few minor string changes: - Capture all in promiscuous mode - Always use promiscuous mode. - Start - Start Capture == 2. Output == This tab replaces the Capture File(s) section of the current Capture Options dialogue. The Use pcap-ng format checkbox becomes a drop-down list for output format, allowing us to add other file formats if we want, and making it clear what the current alternative to pcap-ng is. The implicit temp file used when the File field is blank becomes an explicit check-box. I also added an option to create a new file every N packets in order to be consistent with the Stop capture after options. Also made some more string changes, mostly for clarity and to avoid technical terms like Ring buffer. == 3. Options == This tab replaces the other sections of the current Capture Options dialogue (Display Options, Name Resolution, and Stop Capture...). The only real change (besides more string changes) is that the Stop capture option gets a master checkbox which controls the availability of three condition checkboxes. Cheers, Evan [1] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/1.%20Input.jpg [2] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/2.%20Output.jpg [3] https://dl.dropbox.com/u/171647/Wireshark-Wireframes/3.%20Options.jpg On Fri, Dec 7, 2012 at 9:15 PM, Evan Huus eapa...@gmail.com wrote: On Fri, Dec 7, 2012 at 3:46 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 7, 2012, at 9:06 PM, Evan Huus wrote: On Fri, Dec 7, 2012 at 2:41 PM, Michael Tuexen michael.tue...@lurchi.franken.de wrote: On Dec 7, 2012, at 8:01 PM, Evan Huus wrote: On Fri, Dec 7, 2012 at 1:47 PM, Gerald Combs ger...@wireshark.org wrote: For the Qt toolbar I created start and stop capture icons based on the media player/recorder record (circle) and stop (square) conventions[1][2]. Record makes more sense to me; we are recording packets to disk after all. It also makes things easier if we ever get around to adding a playback feature. My versions are capture_start_24.png, capture_start_active_24.png, and capture_stop_24.png in the image directory. +1 A media-player-ized capture options icon could be a record button with a superimposed wrench. I'm not sure about the interface list or restart capture buttons however. +1 for the capture options icon. I would suggest that (in qtshark) the entire interface list dialog be merged into the capture options dialog - there's a large amount of information duplicated between them and they do almost the same thing already. The dialog should generally be rethought at the same time, as the current capture options dialog is already quite busy -- perhaps splitting it into tabs is the way to go? With the dialogues Hi Evan, the capture options dialog was rethought for 1.8 to support the capturing from multiple interfaces. We wanted to clean things up on the one hand side, don't change too much on the other. That was already in trunk when I first started hacking on Wireshark :) .. OK. I'm not too familiar with the old (1.6?) dialog, but after a quick glance at some old documentation screenshots it looks like the 1.8 version is already a lot cleaner than it was. So if you have concrete suggestions how to improve the capture options dialog box, Irene and myself will be more
[Wireshark-dev] Buildbot version number in fuzz-test bugs?
In cases like bug 8045 [1], it would have been handy for it to say in the report somewhere which build-bot (version and OS) had found the error... is that reasonably easy to do? Thanks, Evan [1] https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8045 ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46320: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-bthci_acl.c
This sounds like something wmem could solve - if there is an intermediate scope between ep and se in duration then it should be possible to create another wmem scope for use here. Is the needed scope for these addresses well defined? On Sat, Dec 1, 2012 at 9:46 PM, morr...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=46320 User: morriss Date: 2012/12/01 06:46 PM Log: Fix https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8030 : Don't store an AT_STRINGZ address in ep_ allocated memory: that memory is freed before the addresses may be used. Use se_ memory instead (no, that's not really ideal either). It would appear that several other dissectors have the same problem. Directory: /trunk/epan/dissectors/ ChangesPath Action +2 -2 packet-bthci_acl.cModified ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org ?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46320: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-bthci_acl.c
On Sat 01 Dec 2012 10:47:47 PM EST, Jeff Morriss wrote: On 12/01/2012 10:23 PM, Evan Huus wrote: This sounds like something wmem could solve - if there is an intermediate scope between ep and se in duration then it should be possible to create another wmem scope for use here. Yeah, I thought so too but I took the quick way out (to shut the buildbot up). Is the needed scope for these addresses well defined? I'm not entirely sure, but it is (apparently) at least the old ep_ scope (in that the memory needs to be around at least until we *start* dissecting the next packet). At least one of the dissectors I looked at used (non-static!) local variables to hold their AT_STRINGZ. :-( Ouch. I'm not sure either, but I think the addresses might need to stick around a little longer than that - maybe the length of the current dissection (ie until the file is loaded or the filter has finished running etc). That strikes me as a nice middle-ground between ep and se, although it may be unnecessarily long for this particular case. Where exactly were the addresses being used that was after the ep memory was freed? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] fuzz failures not generating bugs
On Fri, Nov 30, 2012 at 12:17 PM, Jeff Morriss jeff.morriss...@gmail.comwrote: Hi Gerald, The fuzz-bot seems to be generating fuzz failures but they're not showing up as bugs. For example the latest fuzz failure http://buildbot.wireshark.org/**trunk/builders/Clang-Code-** Analysis/builds/1620/steps/**fuzz-menagerie/logs/stdiohttp://buildbot.wireshark.org/trunk/builders/Clang-Code-Analysis/builds/1620/steps/fuzz-menagerie/logs/stdio was copied to the automated captures area: https://www.wireshark.org/**download/automated/captures/** fuzz-2012-11-30-20336.pcaphttps://www.wireshark.org/download/automated/captures/fuzz-2012-11-30-20336.pcap but no bug was generated. I'd been wondering about that. It's probably related to the recent bugzilla upgrade? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RPM and inter-plugin dependencies
On Fri, Nov 30, 2012 at 1:19 PM, Austin Albright chuckbubba...@hotmail.comwrote: -- Alex -- It has only one library dependency, that isn't a fundamental part of Wireshark... sort of... which is the wimax plugin library (wimax.so) When I run ldd on the customPlugin.so on my development system and then on the version installed via RPM on the production system the results are identical. This is odd - if wimax.so is listed in the output of ldd then the linker should pull in the wimax plugin when yours is loaded, and should be able to resolve the symbol. If you list the symbols exported in wimax.so is get_tlv_value_offset among them? -- Japp -- It is a detailed error message that tells the exact function it can't seem to find. Which is what leads me to think it can't see\access the wimax.so plugin library. The error message is: Couldn't load module /usr/lib/wireshark/plugins/1.6.9pre1_42613/customPlugin.so: /usr/lib/wireshark/plugins/1.6.9pre1_42613/customPlugin.so: undefined symbol: get_tlv_value_offset Where get_tlv_value_offset only exist in the wimax plugin (wimax_tlv.c\h). For some reason, when I began creating this plugin I thought 1.6.8/9 was the last release of the 1.6 trunk. I never even thought to check for updates until I checked the mailing li st to see if my message went through and there was announcement about the latest release/updates to the 1.6 trunk. -- New thought\question -- Should I add something akin to a *target_link_libraries* in the CMakeList.txt pointing to the wimax plugin's library? But then I wonder how it ever compiled if it need that to be specified in the CMakeList file? Since wimax.so is already listed by ldd it sounds like you're already linking against the wimax plugin... Plugins are built as shared libraries (.so files) such that external symbols are not resolved at link time. You could call a non-existent function my_imaginary_function() and it will still compile as long as you've declared a prototype for it, even if there is no definition of it anywhere. The problem seems related to the RPM compilation\build process. But since everything is re-built to create the RPM and I receive no warnings about an undefined function during the RPM's compilation stage... It's possible that the RPM build is stricter about stripping symbols than the default in-tree build. Perhaps you need to add get_tlv_value_offset to a list of exported symbols somewhere? (not too familiar with RPM, sorry) Thank you Alex and Japp for your suggestions. Austin -- From: jaap.keu...@xs4all.nl Date: Fri, 30 Nov 2012 21:44:53 +0400 To: wireshark-dev@wireshark.org Subject: Re: [Wireshark-dev] RPM and inter-plugin dependencies Hi Austin, Forget about the #include. That 's for the compiler to know which file to include in the compilation. This has nothing to do with the object code you're distributing. You state that there's a detailed error message, but you never quote it. What does it tell you? trunk-1.6 SVN head now is version 1.6.12 now (rolled over to 1.6.13 already) Thanks, Jaap Send from my iPhone On 30 nov. 2012, at 18:43, Austin Albright chuckbubba...@hotmail.com wrote: Wireshark Gurus, First things, first... My development system is RHEL v5.6 and as it doesn't support a new enough version of GTK, my work uses the v1.6.9 SVN head. That said, I have created my own plugin, it builds and works great on the system I've developed it on. However, when I build the RPM, and install the RPM on another machine (identical to my development system minus all the source code) I always get a couldn't load module error. The error message is quite detail and so I know it is because my plugin is using some common functions from the wimax plugin (i.e. wimax_tlv.c). I include the wimax_tlv.h in my source as follows: #include plugins/wimax/wimax_tlv.h While grasping at straws I even tried: #include plugins/wimax/wimax_tlv.h Neither #include format made a difference. My build of Wireshark worked regardless on my development system and gave the same exact couldn't load module error on the production system. I don't get any warnings or complaints about the #include plugins/wimax/wimax_tlv.h on my development system when compiling or building the RPM, but it sure seems to have a problem with it on the production system. One other notable thing is that on my development system when I start Wireshark and look at the plugins listing (Help-About-Plugins) my plugin is listed, but on the production system where Wireshark was installed using my RPM, it is not listed in the plugins listing. I followed the directions for creating and including a plugin in README.plugin, but obviously I've missed something. Also, my plugin library does get placed in /usr/lib/wireshark/plugin/version/ on the production system when I install my RPM. I really don't want to have a redundant copy of the code
Re: [Wireshark-dev] fuzz failures not generating bugs
On Fri, Nov 30, 2012 at 3:50 PM, Gerald Combs ger...@wireshark.org wrote: On 11/30/12 12:01 PM, Bill Meier wrote: Assuming that the conversion script mentioned in https://bugzillaupdate.wordpress.com/2010/07/06/bugzilla-4-0-has-a-new-default-status-workflow/ will be run, it appears that the changes in the current status values will be as follows: “NEW” will become “CONFIRMED” “ASSIGNED” will become “IN_PROGRESS” “REOPENED” will become “CONFIRMED” (and the “REOPENED” status will be removed) “CLOSED” will become “VERIFIED” (and the “CLOSED” status will be removed) That's correct. Also, I'm guessing that, after the update, the initial status of a bug will now be CONFIRMED (which corresponds with our current initial status of New). Or: will we now start with UNCONFIRMED ? UNCONFIRMED has to be enabled in the configuration, otherwise the initial status is CONFIRMED. That being said, I can imagine that starting with Confirmed might cause some puzzlement from those used to seeing NEW as the initial status. Would UNCONFIRMED be less confusing than CONFIRMED? I would think so. It's bothered me for a while that we didn't have a way to distinguish between brand new, nobody has looked at it yet bugs and solution identified, but nobody wants to work on it bugs. Separating our current NEW bugs into either UNCONFIRMED or CONFIRMED states seems like the right way to do that. While on the topic, I'd also love an INCOMPLETE state like Launchpad (for bugs that are waiting on the submitter for more information -- we seem to have a fair number of those), but I suppose one thing at a time :) Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] fuzz failures not generating bugs
On Fri, Nov 30, 2012 at 4:34 PM, Bill Meier wme...@newsguy.com wrote: On 11/30/2012 4:08 PM, Evan Huus wrote: Would UNCONFIRMED be less confusing than CONFIRMED? I would think so. It's bothered me for a while that we didn't have a way to distinguish between brand new, nobody has looked at it yet bugs and solution identified, but nobody wants to work on it bugs. Separating our current NEW bugs into either UNCONFIRMED or CONFIRMED states seems like the right way to do that. +1 I would also note that presumably the status can just go from UNCONFIRMED to RESOLVED if a bug is just immediately fixed upon reviewing. I would hope so. While on the topic, I'd also love an INCOMPLETE state like Launchpad (for bugs that are waiting on the submitter for more information -- we seem to have a fair number of those), but I suppose one thing at a time :) +1, I think. How does the incomplete status get updated when the additional information is provided ? manually ? If manually, is this OK in practice or do peole forget to update the status ? The submitter sets it back to NEW (or UNCONFIRMED) manually when they provide the requested information. People don't forget, because bugs in this state expire after 60 days otherwise. It's a great method for keeping the bug list short :) ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] fuzz failures not generating bugs
On Fri, Nov 30, 2012 at 4:35 PM, Guy Harris g...@alum.mit.edu wrote: On Nov 30, 2012, at 1:08 PM, Evan Huus eapa...@gmail.com wrote: I would think so. It's bothered me for a while that we didn't have a way to distinguish between brand new, nobody has looked at it yet bugs and solution identified, but nobody wants to work on it bugs. CONFIRMED is somebody's looked at it and determined that it's really a bug (as opposed to, for example, that's what it's supposed to do); IN_PROGRESS is solution identified and somebody's working on it. There's no separate solution identified, but nobody wants to work on it, or even yes, it's a bug, but we haven't figured out the cause yet state; CONFIRMED covers those two. Yes, this is more precise than my definition. Launchpad provides TRIAGED which is confirmed with enough information to fix, but nobody is working on it yet, but I haven't found it all that useful a state. Separating our current NEW bugs into either UNCONFIRMED or CONFIRMED states seems like the right way to do that. While on the topic, I'd also love an INCOMPLETE state like Launchpad (for bugs that are waiting on the submitter for more information -- we seem to have a fair number of those), but I suppose one thing at a time :) Yes - a bug database I've worked with a state like that. It also had resolutions along the lines of NOTABUG - that's what the software's supposed to do; NOTOURBUG - it's a bug in some other software that we use (and that we can't or shouldn't work around) These would be nice. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 46087: /trunk/ /trunk/: capture_opts.c
On Mon, Nov 19, 2012 at 4:00 PM, morr...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=46087 User: morriss Date: 2012/11/19 01:00 PM Log: I'm pretty confident the string length will fit in 32 bits; cast away the possible loss of data warning on the Win64 build. Directory: /trunk/ ChangesPath Action +11 -11capture_opts.cModified Not that I really think the string is likely to exceed 2^32 characters, but wouldn't it be saner to declare prefix_length as a size_t instead? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] State of wmem
While wmem is still nowhere near complete (it doesn't have an mmap block allocator, it doesn't have a red-black tree implementation), as of revision 45989 it should be able to replace emem in many cases. I've put some documentation in doc/README.wmem which should explain its design and usage in sufficient detail. At this point I'm happy enough with it to recommend it for more general usage (consider it a public beta or something along those lines). For the time being emem is still listed in README.developer, but if no major flaws show up in the next few weeks then I'm ready to switch that over and officially deprecate emem. Please let me know if any of the documentation is unclear. Thanks, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] ATM over Ethernet
On Tue, Nov 6, 2012 at 12:47 PM, Alex Bennée kernel-hac...@bennee.comwrote: On 6 November 2012 10:13, Alex Bennée kernel-hac...@bennee.com wrote: On 5 November 2012 18:53, Guy Harris g...@alum.mit.edu wrote: On Nov 5, 2012, at 4:14 AM, Alex Bennee kernel-hac...@bennee.com wrote: No, you need to implement a dissector for your ATM-over-Ethernet encapsulation that repeatedly calls the ATM dissector for each cell. Multiple cells encapsulated in an Ethernet packet is a characteristic of your encapsulation, not of ATM, so the code to handle that should be part of the dissector for your encapsulation, not the dissector for ATM. Ahh it makes more sense now. I've attached the re-worked patch which adds an explicit atmoe dissector which seems to work. Just something to note - you shouldn't be calling other dissectors inside an if (tree) block. All dissectors down the stack need to be run even if tree is NULL so that conversations, expert info and other non-tree data still gets populated. The various proto_* functions all handle NULL trees and behave correctly, so the easiest way to fix this would be to simply remove the if (tree) check and leave everything else the way it is. Also, (since I'm feeling super picky today) you don't need to #include epan/prefs.h :) Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45189: /trunk/ /trunk/: cfile.h file.c
On Sun, Nov 4, 2012 at 3:34 AM, Jakub Zawadzki darkjames...@darkjames.plwrote:
On Sat, Nov 03, 2012 at 07:55:51PM -0400, Evan Huus wrote:
On Mon, Oct 8, 2012 at 7:12 AM, Jakub Zawadzki
darkjames...@darkjames.pl wrote:
+ if (cf-count frames_count framenum = cf-count) {
+/* XXX, what we should do when new frames were received during
rescaning but user clicked abort?
+ * - call packet_list_append() for all new frames?
+ * - just warn user?
+ */
+ }
Just looking at this for the first time, but shouldn't the first part
of the conditional be cf-count frames_count?
Yes, thanks.
To answer the actual question though - I don't think we should do
anything. Existing frames that we haven't processed yet are simply not
displayed when the user hits abort, so I think it's perfectly sane for
us to not display any new frames in that case either.
But if we don't call packet_list_append() on them, these packets won't
show up on packet list, even if user refilter again and this time he/she
won't abort the process.
Ah, right. In this case (based only on reading the code) I believe it would
be sane to set fdata-flags.passed_dfilter to FALSE on all the new frames
and then call packet_list_append_record() directly. That should add them to
the list without touching any of the GUI stuff.
On the other hand - I'm not sure if they'll have been dissected by this
point. It might be necessary to call add_packet_to_packet_list() instead,
so that they get dissected even if they're hidden. It would have to be
called with some faked dfilter_t that returns false on all packets - does
such a thing exist?
You're right, this is a more complicated question that it first appears to
be.
Evan
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] RFD: The Future of Memory Management in Wireshark
On Sun, Oct 28, 2012 at 10:12 AM, Evan Huus eapa...@gmail.com wrote:
On Sat, Oct 27, 2012 at 3:09 PM, Evan Huus eapa...@gmail.com wrote:
On Sat, Oct 27, 2012 at 2:17 PM, Dirk Jagdmann d...@cubic.org wrote:
General thoughts from the list on whether or not this would be a good idea?
some general comments on the whole wmem idea:
memory allocation is done almost everywhere in Wireshark, also in somewhat
hidden places. For example all the proto_tree_add_* functions will need to
allocate memory. It will be a real pain to pass down the current allocator
object into each of those function invocations, you're looking at changing a
considerable amount of code.
Yes, it wouldn't be a small job.
Also when thinking about writing dissector code,
it's probably not useful to mix different allocators while dissecting a
protocol.
Not sure what you mean here. Mixing ep_ and se_ memory? Or mixing
g_alloc and mmap memory? The separation of the allocators (glib, mmap,
etc.) from the front-end isn't driven by a desire to mix and match
allocators during a specific dissection (I agree that would be odd).
It was more driven by the fact that the current allocators are all
jumbled together and thus hard to maintain. It also makes it easy to
add new scopes where necessary without writing hundreds of new wrapper
functions.
An alternative idea (although a little more ugly) is to use a thread local
global variable which contains pointers to the current ep and se
allocators. The
se_* and ep_* functions retrieve the allocator object from that thread local
global variable and do their allocations. If a function wants to change the
allocators for its own function calls, it can make a copy of the global
variable
pointers to local variables, set it's desired new allocators and restore the
global pointers at the end, like:
__thread void* se_allocator;
__thread void* ep_allocator;
void my_special_func()
{
void* old_se = se_allocator;
void* old_ep = ep_allocator;
se_allocator = create_special_se();
ep_allocator = create_special_ep();
dissect_packet();
garbage_collect(se_allocator);
garbage_collect(ep_allocator);
se_allocator = old_se;
pe_allocator = old_ep;
}
This would certainly require fewer changes to existing code. It is,
however, complicated by the lack of nice cross-platform API for
thread-local storage. I just took a quick look at glib's gestures in
this direction and they seem rather limited (a max of 128 thread-local
objects, and they can't be destroyed).
One of the things I was hoping to do by removing the allocators from
the global scope was make it very obvious what the exact scopes of the
various allocators are. Right now ep_ and se_ memory is used in a lot
of places that aren't correct because the code has simply been able to
grab the global. We really don't want anyone using ep_ memory unless
there is actually a packet being dissected, and scoping the ep_ pool
would enforce that nicely.
We might be able to fake the proper scoping using thread-local globals
if we wrap everything in functions that assert the state of a
dissection. Something like:
__thread wmem_allocator_t *packet_scope;
__thread gboolean packet_in_scope = FALSE;
wmem_allocator_t *
wmem_packet_scope(void)
{
g_assert(packet_in_scope);
return packet_scope;
}
void
wmem_start_packet_scope(void)
{
g_assert(!packet_in_scope);
packet_in_scope = TRUE;
}
void
wmem_stop_packet_scope(void)
{
g_assert(packet_in_scope);
packet_in_scope = FALSE;
wmem_free_all(packet_scope);
}
Invalid accesses would still compile, but at least they would throw an
assertion as soon as the code path was hit. Thoughts on something like
this?
I've committed the base for this in revision 45879. Setting up the
packet-level scopes seems to be fine, but as I mentioned in the commit
message, I can't figure out where to put the calls to
wmem_enter_file_scope() and wmem_leave_file_scope(). My first attempt
put them in init_dissection() and cleanup_dissection() in packet.c but
that caused a lot of assertion errors.
Does anyone know the correct place for these calls?
Thanks,
Evan
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] RFD: The Future of Memory Management in Wireshark
On Mon, Oct 29, 2012 at 5:56 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: Hi Evan, On Sun, Oct 28, 2012 at 07:45:10PM -0400, Evan Huus wrote: Tangentially, I haven't noticed if you've checked in a pinfo memory scope yet or not. Is there anything else wmem needs to be able to do for that to work or did you end up going in a different direction? I need some spare time to write it (and some time to read wmem interface once again). I added some interface documentation to doc/README.wmem in revision 45832 that should help. If you want to do it and have some free time, please do :) Maybe this weekend if nobody else has gotten to it by then... Regards, Kuba. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org 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] RFD: The Future of Memory Management in Wireshark
On Sat, Oct 27, 2012 at 3:09 PM, Evan Huus eapa...@gmail.com wrote:
On Sat, Oct 27, 2012 at 2:17 PM, Dirk Jagdmann d...@cubic.org wrote:
General thoughts from the list on whether or not this would be a good idea?
some general comments on the whole wmem idea:
memory allocation is done almost everywhere in Wireshark, also in somewhat
hidden places. For example all the proto_tree_add_* functions will need to
allocate memory. It will be a real pain to pass down the current allocator
object into each of those function invocations, you're looking at changing a
considerable amount of code.
Yes, it wouldn't be a small job.
Also when thinking about writing dissector code,
it's probably not useful to mix different allocators while dissecting a
protocol.
Not sure what you mean here. Mixing ep_ and se_ memory? Or mixing
g_alloc and mmap memory? The separation of the allocators (glib, mmap,
etc.) from the front-end isn't driven by a desire to mix and match
allocators during a specific dissection (I agree that would be odd).
It was more driven by the fact that the current allocators are all
jumbled together and thus hard to maintain. It also makes it easy to
add new scopes where necessary without writing hundreds of new wrapper
functions.
An alternative idea (although a little more ugly) is to use a thread local
global variable which contains pointers to the current ep and se allocators.
The
se_* and ep_* functions retrieve the allocator object from that thread local
global variable and do their allocations. If a function wants to change the
allocators for its own function calls, it can make a copy of the global
variable
pointers to local variables, set it's desired new allocators and restore the
global pointers at the end, like:
__thread void* se_allocator;
__thread void* ep_allocator;
void my_special_func()
{
void* old_se = se_allocator;
void* old_ep = ep_allocator;
se_allocator = create_special_se();
ep_allocator = create_special_ep();
dissect_packet();
garbage_collect(se_allocator);
garbage_collect(ep_allocator);
se_allocator = old_se;
pe_allocator = old_ep;
}
This would certainly require fewer changes to existing code. It is,
however, complicated by the lack of nice cross-platform API for
thread-local storage. I just took a quick look at glib's gestures in
this direction and they seem rather limited (a max of 128 thread-local
objects, and they can't be destroyed).
One of the things I was hoping to do by removing the allocators from
the global scope was make it very obvious what the exact scopes of the
various allocators are. Right now ep_ and se_ memory is used in a lot
of places that aren't correct because the code has simply been able to
grab the global. We really don't want anyone using ep_ memory unless
there is actually a packet being dissected, and scoping the ep_ pool
would enforce that nicely.
We might be able to fake the proper scoping using thread-local globals
if we wrap everything in functions that assert the state of a
dissection. Something like:
__thread wmem_allocator_t *packet_scope;
__thread gboolean packet_in_scope = FALSE;
wmem_allocator_t *
wmem_packet_scope(void)
{
g_assert(packet_in_scope);
return packet_scope;
}
void
wmem_start_packet_scope(void)
{
g_assert(!packet_in_scope);
packet_in_scope = TRUE;
}
void
wmem_stop_packet_scope(void)
{
g_assert(packet_in_scope);
packet_in_scope = FALSE;
wmem_free_all(packet_scope);
}
Invalid accesses would still compile, but at least they would throw an
assertion as soon as the code path was hit. Thoughts on something like
this?
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] RFD: The Future of Memory Management in Wireshark
On Sun, Oct 28, 2012 at 7:26 PM, Jakub Zawadzki
darkjames...@darkjames.pl wrote:
On Sun, Oct 28, 2012 at 10:12:41AM -0400, Evan Huus wrote:
We might be able to fake the proper scoping using thread-local globals
if we wrap everything in functions that assert the state of a
dissection. Something like:
__thread wmem_allocator_t *packet_scope;
__thread gboolean packet_in_scope = FALSE;
Just a note before commiting:
__thread is GCC extension, MVSC has got __declspec(thread) and
C11 has got _Thread_local.
So to be portable we must use glib GStaticPrivate.
Yes, I just wanted to be consistent with Dirk's previous example.
wmem_allocator_t *
wmem_packet_scope(void)
{
g_assert(packet_in_scope);
return packet_scope;
}
void
wmem_start_packet_scope(void)
{
g_assert(!packet_in_scope);
packet_in_scope = TRUE;
}
void
wmem_stop_packet_scope(void)
{
g_assert(packet_in_scope);
packet_in_scope = FALSE;
wmem_free_all(packet_scope);
}
Invalid accesses would still compile, but at least they would throw an
assertion as soon as the code path was hit. Thoughts on something like
this?
I'm not sure if you want to replace all ep_ pool with packet_scope pool,
but if yes what kind of memory you want to use in GUI, or for error messages
in epan/dfilter/? glib allocator?
I think the current scope for ep_ memory is basically correct now,
thanks to your work moving ep_free_all to after dissect_packet. The
epan_dissect_run and epan-dissect_run_with_taps functions would
basically be:
wmem_start_packet_scope();
dissect_packet();
wmem_stop_packet_scope();
ep_ memory that is currently being allocated outside of that scope
would have to be converted on a case by case basis - I suspect some of
it would be trivial to safely convert to glib. As Jeff pointed out in
your thread, preventing leaks when exceptions are thrown is the
biggest win for emem and wmem, so if it's being used in a scope where
exceptions aren't thrown then it should be sane (if not easy) to
convert to glib. Other cases (liked dfilter) might warrant having its
own internal scope, which wmem makes it possible to do sanely.
Tangentially, I haven't noticed if you've checked in a pinfo memory
scope yet or not. Is there anything else wmem needs to be able to do
for that to work or did you end up going in a different direction?
Cheers,
Evan
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] RFD: The Future of Memory Management in Wireshark
On Fri, Oct 26, 2012 at 12:14 PM, Sébastien Tandel sebastien.tan...@gmail.com wrote: On Fri, Oct 26, 2012 at 1:58 PM, Evan Huus eapa...@gmail.com wrote: On Fri, Oct 26, 2012 at 11:40 AM, Graham Bloice graham.blo...@trihedral.com wrote: On 26 October 2012 14:44, Evan Huus eapa...@gmail.com wrote: On Fri, Oct 26, 2012 at 9:29 AM, Sébastien Tandel sebastien.tan...@gmail.com wrote: On Wed, Oct 24, 2012 at 11:13 AM, Evan Huus eapa...@gmail.com wrote: On Wed, Oct 24, 2012 at 8:10 AM, Sébastien Tandel sebastien.tan...@gmail.com wrote: On Wed, Oct 24, 2012 at 1:10 AM, Guy Harris g...@alum.mit.edu wrote: On Oct 18, 2012, at 6:01 PM, Evan Huus eapa...@gmail.com wrote: I have linked a tarball [2] containing the following files: - wmem_allocator.h - the definition of the allocator interface - wmem_allocator_glib.* - a simple implementation of the allocator interface backed by g_malloc and a singly-linked list. Presumably an implementation of the allocator could, instead of calling a lower-level memory allocator (malloc(), g_malloc(), etc.) for each allocation call, allocate larger chunks and parcel out memory from the larger chunks (as the current emem allocator does), if that ends up saving enough CPU, by making fewer allocate and free calls to the underlying memory allocator, so as to make it worth whatever wasted memory we have at the ends of chunks? One step further, instead of mempools, I think wireshark could have great interest in implementing slabs (slab allocator). Slabs had initially been designed for kernel with several advantages over traditional allocators in terms of resources needed to allocate (CPU), (external / internal) fragmentation and also cache friendliness (most of the traditional allocators don't care). I've attached some slides about a high-level description of slab. Since then, another paper has been written showing some improvements and what it took to write a slab for user-space (libumem). There is another well-known exampel out there, called memcache, that implements its own version (and could be a good intial point for wireshark implementation, who knows? :)) If I understand correctly, a slab allocator provides the most benefit when you have to alloc/free a large number of the same type of object, you're right, that's where slab is the most efficient at. Although, the second paper shows it can be efficient for general purpose allocation based on size and not specific structure. but I don't know if this is necessarily the case in Wireshark. There are probably places where it would be useful, but I can't think of any off the top of my head. TVBs maybe? I know emem is currently used all over the place for all sorts of different objects... I guess the most obvious would be emem_tree (emem_tree_node) might be an example used all over and over while dissecting. :) There is indeed a bunch of different objects allocated with emem. Also, it might be used to allocate memory for some fragments. Ah, yes, the various emem data structures (tree, stack, etc.) would likely benefit from slab allocators. Converting them to use slabs would be something to do while porting them from emem to wmem. Since your interface seems to allow it, we could create several slabs types, one for each specific structures that are allocated very frequently (emem_tree_node?), others for packets/fragments with some tuned slabs sizes and another with some generic sizes. That seems reasonable, presumably with some shared slab code doing the type-agnostic heavy lifting. I'll have to give a bit of thought to what the interface for that would be like - if you already have an interface in mind, please share :) Are the slab allocators mentioned homegrown or provided by the host OS. If the latter, what platforms are they available on? Homegrown on top of malloc/g_malloc/mmap, I believe. A slab allocator is (or was) used internally in the linux and solaris kernels, but has never been exposed to userspace to my knowledge. It's indeed not exposed to users. It's used internally as a kernel object cache allocator. But, memcached has a user-space implementation that could -probably- be leveraged for wireshark. I took a quick look, and I think it would be significant overkill for our needs. It also directly references pthreads a lot, which isn't available to us on Windows. It might be useful as a reference implementation, but I don't think it's worth using directly. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http
Re: [Wireshark-dev] RFD: The Future of Memory Management in Wireshark
On Wed, Oct 24, 2012 at 8:10 AM, Sébastien Tandel sebastien.tan...@gmail.com wrote: On Wed, Oct 24, 2012 at 1:10 AM, Guy Harris g...@alum.mit.edu wrote: On Oct 18, 2012, at 6:01 PM, Evan Huus eapa...@gmail.com wrote: I have linked a tarball [2] containing the following files: - wmem_allocator.h - the definition of the allocator interface - wmem_allocator_glib.* - a simple implementation of the allocator interface backed by g_malloc and a singly-linked list. Presumably an implementation of the allocator could, instead of calling a lower-level memory allocator (malloc(), g_malloc(), etc.) for each allocation call, allocate larger chunks and parcel out memory from the larger chunks (as the current emem allocator does), if that ends up saving enough CPU, by making fewer allocate and free calls to the underlying memory allocator, so as to make it worth whatever wasted memory we have at the ends of chunks? One step further, instead of mempools, I think wireshark could have great interest in implementing slabs (slab allocator). Slabs had initially been designed for kernel with several advantages over traditional allocators in terms of resources needed to allocate (CPU), (external / internal) fragmentation and also cache friendliness (most of the traditional allocators don't care). I've attached some slides about a high-level description of slab. Since then, another paper has been written showing some improvements and what it took to write a slab for user-space (libumem). There is another well-known exampel out there, called memcache, that implements its own version (and could be a good intial point for wireshark implementation, who knows? :)) If I understand correctly, a slab allocator provides the most benefit when you have to alloc/free a large number of the same type of object, but I don't know if this is necessarily the case in Wireshark. There are probably places where it would be useful, but I can't think of any off the top of my head. TVBs maybe? I know emem is currently used all over the place for all sorts of different objects... You could certainly shoehorn a slab allocator into wmem's current architecture, but you'd have to abuse the wmem_allocator_t interface to do it. I suspect that it would make more sense to implement slabs separately anyways - since their goal is primarily performance you would want to cut out the function pointers that wmem currently uses. It's definitely worth thinking about though. Thanks, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] wmem allocator and GLIB version
On Wed, Oct 24, 2012 at 8:08 AM, Pascal Quantin pascal.quan...@gmail.com wrote: Hi all, wmem_glib_free_all() makes use of g_slist_free_full that was introduced in GLIB 2.28 while our minimum requirement is GLIB 2.14.0. Could we manually free list elements containing dynamically-allocated memory and call g_slist_free() instead to keep compatibility with older GLIB versions? Regards, Pascal. Oops, I didn't realize g_slist_free_full was that recent. I'll add it to my todo list. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: The Future of Memory Management in Wireshark
On Tue, Oct 23, 2012 at 11:10 PM, Guy Harris g...@alum.mit.edu wrote: On Oct 18, 2012, at 6:01 PM, Evan Huus eapa...@gmail.com wrote: I have linked a tarball [2] containing the following files: - wmem_allocator.h - the definition of the allocator interface - wmem_allocator_glib.* - a simple implementation of the allocator interface backed by g_malloc and a singly-linked list. Presumably an implementation of the allocator could, instead of calling a lower-level memory allocator (malloc(), g_malloc(), etc.) for each allocation call, allocate larger chunks and parcel out memory from the larger chunks (as the current emem allocator does), if that ends up saving enough CPU, by making fewer allocate and free calls to the underlying memory allocator, so as to make it worth whatever wasted memory we have at the ends of chunks? Absolutely: this is one of the things on my to-do list for wmem. I haven't started it yet though. so if somebody else wants to grab it that would be great. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Limiting scope of ep_ memory
On Mon, Oct 22, 2012 at 12:21 PM, Anders Broman anders.bro...@ericsson.com wrote: -Original Message- From: wireshark-dev-boun...@wireshark.org [mailto:wireshark-dev-boun...@wireshark.org] On Behalf Of Jakub Zawadzki Sent: den 22 oktober 2012 09:11 To: Developer support list for Wireshark Subject: Re: [Wireshark-dev] RFD: Limiting scope of ep_ memory On Tue, Oct 16, 2012 at 03:18:32PM +, Anders Broman wrote: I think it sounds like the right thing to do and as no one have any objections I think you might as well go ahead and check it in :-) Bug #7892. Some solutions to fix it: 1/ revert r45673 2/ call epan_dissect_fill_in_columns() before ep_free_all() epan_dissect_fill_in_columns() in 8 cases of 9 is called after epan_dissect_run(). Only complicated case is tshark, which pass edt pointer to print_packet(), which later not always call epan_dissect_fill_in_columns(). Still it won't work, if GUI use ep_ allocated addresses. 3/ don't use ep_ memory for pinfo- addresses? Greping for e.g. \net_src\ in GTK+ code shows that it's used also to build conversation filters (ip, ethernet, ...) for these addr-data points to tvb (tvbs are freed in epan_dissect_cleanup()). I haven't (yet) found use of address with AT_STRINGZ data, but it can change anytime. I'm no expert at this but some thoughts: - What is the ep scope e.g lifetime of it. I think the way you defined it looks good so that implies we should fix the problems that pops up. Does any one have other thoughts? If ep lifetime is the problem should we revert back and look at the packet list instead? Some of the optimizations calling for redissection is perhaps overkill or would it help to dissect a few more packets than the visible ones when scrolling. - Isn't all these problems latent in the code and could crop up at any time someting changes? It's code that has been written to rely on the fact that ep memory didn't used to be freed until quite late. It's not necessarily *wrong* (although one could argue that the old ep behaviour was wrong) but it certainly won't work with the way ep memory is now being freed. - I think the whole ep/se memory idea is optimizing for execution speed and as a bonus fewer memory leaks Does this assumption still hold true e.g. ep_ is faster that g_malloc? (Changing back would be a nightmare I suppose). But it seems like having ep memory is geting quite complicated. I think reducing memory leaks is the primary goal, and execution speed was a bonus. I agree that it's getting overly complicated though, see my email from last week [1] for some thoughts on where I'd like to go long-term. - My feeling is that mixing ep and g_malloced memory in pinfo is a bad idea. Yes. Perhaps pinfo should have its own scope that is between ep and se? This would be much easier given the changes suggested in [1]. Cheers, Evan [1] https://www.wireshark.org/lists/wireshark-dev/201210/msg00178.html ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Limiting scope of ep_ memory
On Mon, Oct 22, 2012 at 5:36 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Mon, Oct 22, 2012 at 12:43:49PM -0400, Evan Huus wrote: Perhaps pinfo should have its own scope that is between ep and se? This would be much easier given the changes suggested in [1]. [1] https://www.wireshark.org/lists/wireshark-dev/201210/msg00178.html Evan, when do you plan to merge it? Well, I've had absolutely zero feedback on it so far, so I'm not sure if that's a good sign or a bad one... I was planning on taking it slow, maybe merging it this weekend if nobody raised any objections. I can merge it basically right now though if you can use it - since it doesn't touch any existing code it's not that disruptive. I just didn't want to merge it before the general design had gotten a couple of acks on the list, since API changes after it's in use are a pain. I think I need something simple: pi_*(packet_info *pinfo, ...) which just use glib/system allocator, and store all pointers in list (pinfo-gc_list). Later memory would be freed in epan_dissect_cleanup(). But I can wait for wmem. A simple glib+list allocator is already available in the wmem I linked in the email, so I think it would be pretty trivial to have a pinfo-pool, allocate everything in that pool and then just free the pool in epan_dissect_cleanup(). Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] RFD: The Future of Memory Management in Wireshark
TL;DR - Jakub recently proposed a few changes to emem [1]. While I think they are a very good idea, I believe that in the long term the current emem design has too many fundamental limitations to make it worth adapting for our future needs. I propose that it should be gradually deprecated in favour of something else, and I have written a simple example of what that something else might be. --- The Long Version --- After my recent adventures with emem, I took some time to work through what features Wireshark will likely want in its memory manager now and in the future. Predicting the future is always a tricky business, but I tried not to stray too far from the obvious: - Arbitrary instances of the current seasonal and ephemeral allocators. These will be necessary if we ever want to support opening multiple files at once. - Thread-safe allocators. Necessary if we ever want to do multi-threaded dissection or re-dissection of a single file. - Allocators with different scopes. I can think of a couple of different places where a new allocator scope might simplify existing code, and I'm sure there are others. With these ideas in mind, I then took a closer look at the current emem design and tried to estimate the amount of work involved in adapting (and maintaining) it going forward. I concluded that in the long run, emem has too many fundamental limitations to make it worth adapting: - Supporting arbitrary seasonal and ephemeral pools would require non-trivial API changes, causing a lot of pain in dependent code. - The current glib and mmap allocators are jammed together, often living side-by-side in the same functions. As I found out recently, trying to tweak either one can have a lot of unexpected side-effects. - Adding a single new scope requires writing a new wrapper for every API function (*_alloc, *_alloc0, *_strdup, *_strndup, etc). At best this is just extra work, but at worst it leads to API inconsistencies, where, for example, there's an ep_strbuf implementation but no equivalent se_ functions (that's a real example by the way). I would like to propose that emem is gradually deprecated in favour of a new memory management framework that is designed with these requirements in mind. A separate new interface will ease the pain of migration by allowing us to maintain emem as a deprecated interface for as long as necessary. In the spirit of backing up ideas with working code, I have written a simple version of what I think such a new framework might look like. It cleanly separates out the allocator back-ends (glib, mmap, etc) and the utility functions (strdup etc.) from the core interface. It also requires that all API functions are explicitly passed the scoped allocator instance they want to use. This makes it trivial to add new scopes or to support multiple instances of the current scopes. Because the allocators are cleanly distinguished, making any one of them thread-safe is a fairly simple operation. I have linked a tarball [2] containing the following files: - wmem_allocator.h - the definition of the allocator interface - wmem_allocator_glib.* - a simple implementation of the allocator interface backed by g_malloc and a singly-linked list. - wmem_core.* - implementations of wmem_alloc() and wmem_alloc0() - wmem_strutl.* - implementations of wmem_strcpy() and wmem_strncpy() - wmem.h - the general header file for inclusion by consumers of wmem, it simply wraps inclusion of wmem_core.h, wmem_strutl.h and any others that might be created The usage might look something like this: wmem_allocator_t *ep_scope = wmem_create_glib_allocator(); doWork(ep_scope); wmem_destroy_glib_allocator(ep_scope); and then in doWork, instead of ep_alloc(numBytes) you would call wmem_alloc(ep_scope, numBytes). Alternatively, if the outer block is in a loop then it doesn't have to create/destroy each time, and can simply call wmem_free_all(ep_scope) between calls to doWork(). Ideas, comments and constructive criticisms are always welcome. What do you think? Evan [1] https://www.wireshark.org/lists/wireshark-dev/201210/msg00116.html [2] https://dl.dropbox.com/u/171647/wmem.tar.gz ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Add date to release notes
I recently wanted to find out when a specific point release was made available, and I didn't have it installed to check the build time, so I went to the release notes online [1], but that doesn't actually give a date. I ended up having to dig through the wireshark-announce archives to find out. It would be nice if future release notes had a datestamp in them - I expect this could be automated as part of the release process, but I have no idea how? Thanks, Evan [1] http://www.wireshark.org/docs/relnotes/ ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] initializing a variable with a non-constant value
On Sun, Oct 14, 2012 at 1:36 PM, Martin Kaiser li...@kaiser.cx wrote: Hi, doc/README.developer says Don't initialize variables in their declaration with non-constant values. Not all compilers support this. E.g. don't use guint32 i = somearray[2]; ... In file.c, read_packet(), we do const struct wtap_pkthdr *phdr = wtap_phdr(cf-wth); union wtap_pseudo_header *pseudo_header = wtap_pseudoheader(cf-wth); const guchar *buf = wtap_buf_ptr(cf-wth); The two things are in contradiction and I'm wondering how to fix this. Apparently, there's been no complaints about compiler problems with read_packet() so I was wondering if the limitation in README.developer is still required. Any views about this? Or any idea which compilers could potentially be affected? Thanks, Martin This is very similar in theme to my question about variadic macros a few weeks ago [1]. The conclusion there was that they're not officially part of C89 which is Wireshark's official C version. Even though it seems all of the compilers we use support that particular C99 extension, we can't move to C99 officially because MSVC doesn't support all of it, so it's simpler for everyone if we try to stick to pure C89. It's obviously not a big deal if the occasional C99 slips in accidentally as long as it doesn't cause any build problems, but we should try to avoid it where possible. Cheers, Evan [1] https://www.wireshark.org/lists/wireshark-dev/201209/msg00142.html ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] QtShark now passes CppCheck
Like the subject says, as of revision 45519 QtShark (meaning the /ui/qt/ directory) produces no warnings at all under the latest stable CppCheck. Going forward I'll try to keep it that way :) ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Limiting scope of ep_ memory
On Thu, Oct 11, 2012 at 4:41 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: Hi, Right now ep_ memory is freed at the begining of epan_dissect_run(), which means that pointers allocated by ep_ can be safety accessed only before dissecting next packet. When using GUI epan_dissect_run() can be run when refreshing interface (think: packet list). Which can happen at *any* time. Originally it caused bug #5284. Right now we (I and Evan) can't reproduce this bug, but there's still the problem. To make it reproductable (and clear) I want to make ep_ memory available *only* when dissecting packet, i.e. epan_dissect_run(): ep_free_all(); dissect_packet(edt, pseudo_header, data, fd, cinfo); ep_free_all(); Taps also use ep_ memory, so I propose new function: epan_dissect_run_tap(): ep_free_all(); tap_queue_init(edt); dissect_packet(edt, pseudo_header, data, fd, cinfo); tap_push_tapped_queue(edt); ep_free_all(); (For now I want to have ep_free_all() before and after dissecting, before release we can remove the one before). Thanks to it, and memory scrubbing, any ep_ allocated pointer used after dissecting should be catched fast. It'll be no longer possible to save ep_ pointers in taps [1], or communicate with UI by storing ep_ memory in packet_info. I hope this proposal is understable and sane. If we want to allow using ep_ memory between epan_dissect_init() and epan_dissect_cleanup() we need more complicated allocator, which we currently don't have. It's doable, but I'm not sure if really needed. Regards, Jakub. [1] http://www.wireshark.org/lists/wireshark-dev/201210/msg00094.html +1 I also think we should limit it so it's not possible to use ep_ memory while there isn't a packet being dissected. During my original attempt to create a safer allocator I was forced to add a dummy memory pool to work around the numerous locations that use ep_ memory when there isn't a packet in scope. If there isn't a packet currently in scope we have no guarantees when ep_ memory will next be freed, and so it isn't safe to be used. Perhaps the ep_free_all(); dissect_packet(edt, pseudo_header, data, fd, cinfo); ep_free_all(); in Jakub's patch could be ep_start_packet(); dissect_packet(edt, pseudo_header, data, fd, cinfo); ep_end_packet(); where the two new functions just call ep_free_all() and flip a boolean. If ep_alloc() is called when the boolean is false (there is no packet in scope) then it should g_warning (or assert, but I figure trunk would be unusable for days if we do that). Other thoughts? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-bugs] [Bug 7814] Buildbot crash output: fuzz-2012-10-08-21623.pcap
On Wed, Oct 10, 2012 at 7:13 AM, Martin Mathieson martin.r.mathie...@googlemail.com wrote: On Wed, Oct 10, 2012 at 5:19 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Tue, Oct 09, 2012 at 10:53:41PM -0400, Martin Mathieson wrote: I am getting the same assertion, for every file that I try reload/refilter. Can you get errno number for me? 12 (Cannot Allocate Memory?) Probably caused by, from the man page for mprotect: Addresses in the range [addr, addr+len-1] are invalid for the address space of the process, or specify one or more pages that are not mapped. Could this be caused by trying to mprotect the same region of memory twice? That would never happen in the old allocator, but is certainly a possibility now. Is there a fix in the works? Nope, but I submitted alternative patch in [Bug 7775] Wireshark leaks memory when selecting packets In the meantime, could someone advise which files to rollback to which versions so that trunk is again usable? r45388 epan/emem.[ch] epan/epan.c epan_dissect.h Thanks. If you can't reproduce it there, I'm happy to try patches and let you know what happens. I committed a work-around in revision 45444 that should fix the bug (potentially at a small speed cost) until I have a chance to figure out a proper solution. I'm unfortunately short on time for a few days, so if someone else wants to take a crack at it feel free. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45445: /trunk/epan/ /trunk/epan/: emem.c
On Wed, Oct 10, 2012 at 8:24 AM, darkja...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=45445 User: darkjames Date: 2012/10/10 05:24 AM Log: Fix bug #7814 We need to pass original pointer and length to munmap(). Directory: /trunk/epan/ ChangesPath Action +8 -4 emem.cModified Ahah, this all makes sense to me now, thank you for the fix! Although how did struct emem_chunk_t end up with two different members named org? Is that some intentional type-masking that I don't understand, or is it just a typo? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45445: /trunk/epan/ /trunk/epan/: emem.c
On Wed, Oct 10, 2012 at 8:32 AM, Evan Huus eapa...@gmail.com wrote: On Wed, Oct 10, 2012 at 8:24 AM, darkja...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=45445 User: darkjames Date: 2012/10/10 05:24 AM Log: Fix bug #7814 We need to pass original pointer and length to munmap(). Directory: /trunk/epan/ ChangesPath Action +8 -4 emem.cModified Ahah, this all makes sense to me now, thank you for the fix! Although how did struct emem_chunk_t end up with two different members named org? Is that some intentional type-masking that I don't understand, or is it just a typo? And I spoke too soon, you removed it in rev 45446, sorry. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45445: /trunk/epan/ /trunk/epan/: emem.c
On Wed, Oct 10, 2012 at 8:53 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Wed, Oct 10, 2012 at 08:32:00AM -0400, Evan Huus wrote: On Wed, Oct 10, 2012 at 8:24 AM, darkja...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=45445 User: darkjames Date: 2012/10/10 05:24 AM Log: Fix bug #7814 We need to pass original pointer and length to munmap(). Directory: /trunk/epan/ ChangesPath Action +8 -4 emem.cModified Ahah, this all makes sense to me now, thank you for the fix! Although how did struct emem_chunk_t end up with two different members named org? Is that some intentional type-masking that I don't understand, or is it just a typo? Short story how to make development wireshark paintful. Two machines: A - laptop (faster, without ssh keys) development B - desktop (slower, with ssh keys) commiting A: git repository ## coding, coding, coding.. ## compiles, great. $ git diff /tmp/a.patch B: scp 10.XX:/tmp/a.patch /tmp/ $ patch -p2 /tmp/a.patch patching file emem.c Hunk #2 succeeded at 310 (offset 1 line). Hunk #3 succeeded at 671 (offset 1 line). Hunk #4 succeeded at 683 (offset 1 line). ## WTF, why with offset?! $ svn diff ## looks good $ svn commit typing, typing, typing :wq Committed revision 45445. (looking at commit on wireshark-commit...) Committed revision 45446. Well, main problem is that I don't have building env on machine B, and I can't test if it compiles before commiting... Anyway I hope this fix is temporary, right? The fix is needed regardless of what else happens - even with the old old allocator this was still a bug (in the sl_ allocator, if not the ep_ one), we just weren't using enough pages to hit it. However, once I land proper sharing of free pages between pools then this path will probably stop being hit again (just like before). I don't want to make promises I can't keep, but I have an idea I'm pretty sure will work, and Thursday evening is looking promising for time to implement it. I'll make sure to test with both allocation schemes this time though! Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45445: /trunk/epan/ /trunk/epan/: emem.c
On Wed, Oct 10, 2012 at 9:50 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Wed, Oct 10, 2012 at 09:32:07AM -0400, Evan Huus wrote: The fix is needed regardless of what else happens - even with the old old allocator this was still a bug (in the sl_ allocator, if not the ep_ one) Nah, sl_ allocator don't have this bug. 1. Right now sl_free_all() is never used 2. even if it would be, there's no guard pages so npc-buf is not shifted, and npc-amount_free_init has correct size value. And I'm mainly worried about sl_ allocator, cause with that commit I added another 12 bytes to emem_chunk_t... I land proper sharing of free pages between pools then this path will probably stop being hit again (just like before). I don't want to make promises I can't keep, but I have an idea I'm pretty sure will work, and Thursday evening is looking promising for time to implement it. I'll make sure to test with both allocation schemes this time though! I still think that https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5284#c26 would also fix the problem, and we just unnecessary overcomplicate allocator. Is that not the same idea Guy and Jeff discussed that earlier in the bug (comments 2 through 6)? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-bugs] [Bug 7814] Buildbot crash output: fuzz-2012-10-08-21623.pcap
On Wed, Oct 10, 2012 at 10:51 AM, Martin Mathieson
martin.r.mathie...@googlemail.com wrote:
I have discovered one problem since the change, but it may have been a bug
all along.
In tcp_graph.c, it was referencing the tap (struct tcpheader) after the tap
had run. The struct is allocated in packet-tcp.c using ep_alloc(), but now
it wasn't valid to access that memory (immediately after tap_tcpip_packet()
had returned). gdb reported that it wasn't valid to read that memory
address anymore - is this a result of the change to emem.c?
Yes, although I'm not actually sure which behavior (old or new) is correct.
At this point I want to just revert the whole recent set of emem
changes *and* the ref-counting. The old, simple version has exactly
one known bug that's really hard to trigger and I think is probably a
less serious issue than the memory leak that ref-counting gave us. I
don't have access to a commit-capable machine again until tomorrow
evening, so if somebody else wants to take care of it that would be
appreciated.
Jakub, I still don't understand your proposed fix, but that's almost
certainly my fault, not yours. If it fixes the original crasher and
doesn't cause any other problems, go ahead and commit it.
The fix (which I think I'm happy with) was to take a deep copy of the struct
inside the tap function, i.e.
Index: ui/gtk/tcp_graph.c
===
--- ui/gtk/tcp_graph.c (revision 45446)
+++ ui/gtk/tcp_graph.c (working copy)
@@ -1885,7 +1885,10 @@
/* Add address if unique and have space for it */
if (is_unique (th-num_hdrs MAX_SUPPORTED_TCP_HEADERS)) {
- th-tcphdrs[th-num_hdrs++] = header;
+ /* Need to take a deep copy of the tap struct, it may not be
valid
+ to read after this function returns? */
+ th-tcphdrs[th-num_hdrs] = g_malloc(sizeof(struct
tcpheader));
+ *(th-tcphdrs[th-num_hdrs++]) = *header;
}
This is probably a good idea even with the old allocator.
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] [Wireshark-bugs] [Bug 7814] Buildbot crash output: fuzz-2012-10-08-21623.pcap
On Wed, Oct 10, 2012 at 3:18 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Wed, Oct 10, 2012 at 12:19:42PM -0400, Evan Huus wrote: At this point I want to just revert the whole recent set of emem changes *and* the ref-counting. (...) I don't have access to a commit-capable machine again until tomorrow evening, so if somebody else wants to take care of it that would be appreciated. Done. If it fixes the original crasher and doesn't cause any other problems, go ahead and commit it. I can't reproduce bug #5284 with GTK+ 2.24.4, I'll try wireshark-1.8.x. 1.8 still has ref-counting, so you won't see it there. Last time I looked at it (April) I could reproduce, so if you still can't I'll give it a try. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-bugs] [Bug 7814] Buildbot crash output: fuzz-2012-10-08-21623.pcap
Sent that last one too fast... On Wed, Oct 10, 2012 at 3:19 PM, Evan Huus eapa...@gmail.com wrote: On Wed, Oct 10, 2012 at 3:18 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Wed, Oct 10, 2012 at 12:19:42PM -0400, Evan Huus wrote: At this point I want to just revert the whole recent set of emem changes *and* the ref-counting. (...) I don't have access to a commit-capable machine again until tomorrow evening, so if somebody else wants to take care of it that would be appreciated. Done. Thanks. If it fixes the original crasher and doesn't cause any other problems, go ahead and commit it. I can't reproduce bug #5284 with GTK+ 2.24.4, I'll try wireshark-1.8.x. 1.8 still has ref-counting, so you won't see it there. Last time I looked at it (April) I could reproduce, so if you still can't I'll give it a try. I won't be able to try it until Thursday evening, so if you can't reproduce by then I'll give it a shot. Fortunately 1.8 doesn't have most of the leak that the ref-counting caused, since that was dependent on some other changes, so none of the recent mess affects a released version. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c
On Wed, Oct 10, 2012 at 5:32 PM, Maynard, Chris christopher.mayn...@gtech.com wrote: Hi Evan, I finally got around to applying/testing your patch on Windows XP SP3 32-bit. As expected, Wireshark continues to capture just fine. The relevant code in gui_utils and tshark are very similar indeed, but there are some differences, so I'm not sure if they could be combined or not. But as Guy pointed out in a subsequent response, perhaps pipes no longer need to be handled any differently on Windows anymore and we no longer need to wait for exit status? In truth, I haven't looked at this code too much and it's not really an area I'm too comfortable digging into right now. Frankly, me neither, especially since it's all Windows-specific and I still haven't set up my old Windows VM for development. I've filed a low-priority bug (#7847) so it doesn't get absolutely forgotten, but I'm not too inclined to go stirring up trouble at this point. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Windows XP GTK bundle
According to bug #7775 [1], our current GTK bundle (2.24) has a fairly severe memory leak on Windows XP. It doesn't seem to occur in 2.22, so it may be worth reverting to that version for future 1.8 releases? Thoughts? Evan [1] https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7775 ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] buildbot failure in Wireshark (development) on Solaris-10-SPARC
This looks like it came from revision 45388. I'm not too familiar with Solaris, but I didn't think I changed anything that would require linker changes as well? On Mon, Oct 8, 2012 at 11:36 AM, buildbot-no-re...@wireshark.org wrote: The Buildbot has detected a new failure on builder Solaris-10-SPARC while building Wireshark (development). Full details are available at: http://buildbot.wireshark.org/trunk/builders/Solaris-10-SPARC/builds/2460 Buildbot URL: http://buildbot.wireshark.org/trunk/ Buildslave for this Build: solaris-10-sparc Build Reason: scheduler Build Source Stamp: 45388 Blamelist: eapache,etxrab BUILD FAILED: failed compile sincerely, -The Buildbot ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-bugs] [Bug 5284] new_packet_list: redissection + redraw crashes when multi-data-source packet is selected
On Mon, Oct 8, 2012 at 2:37 PM, Pascal Quantin pascal.quan...@gmail.com wrote: Hi Evan, 2012/10/8 bugzilla-dae...@wireshark.org https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=5284 Evan Huus eapa...@gmail.com changed: What|Removed |Added Status|NEW |RESOLVED Resolution||FIXED --- Comment #28 from Evan Huus eapa...@gmail.com 2012-10-08 08:26:16 PDT --- The last little bits of this have finally been properly fixed in rev 45389. New bugs with that code are new bugs, as this one is getting rather unwieldy :) Now when compiling with gcc-4.3.2 on my old Debian Lenny, I get: make[3]: Entering directory `/home/pascal/soft/wireshark/trunk/epan' /bin/sh ../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I.. -I./.. -I./../tools/lemon -I./wslua -DINET6 -DG_DISABLE_DEPRECATED -DG_DISABLE_SINGLE_INCLUDES -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -D_FORTIFY_SOURCE=2 -I/usr/local/include -I /home/pascal/soft/include '-DPLUGIN_DIR=/usr/local/lib/wireshark/plugins/1.9.0' -Werror -DPYTHONDIR= -g -O2 -Wall -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wpointer-arith -Wno-pointer-sign -Warray-bounds -Wcast-align -Wformat-security -Wold-style-definition -D_REENTRANT -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/directfb -I/usr/include/libpng12 -I/usr/include/pixman-1 -MT libwireshark_la-emem.lo -MD -MP -MF .deps/libwireshark_la-emem.Tpo -c -o libwireshark_la-emem.lo `test -f 'emem.c' || echo './'`emem.c gcc -DHAVE_CONFIG_H -I. -I.. -I./.. -I./../tools/lemon -I./wslua -DINET6 -DG_DISABLE_DEPRECATED -DG_DISABLE_SINGLE_INCLUDES -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -D_FORTIFY_SOURCE=2 -I/usr/local/include -I /home/pascal/soft/include -DPLUGIN_DIR=\/usr/local/lib/wireshark/plugins/1.9.0\ -Werror -DPYTHONDIR= -g -O2 -Wall -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wpointer-arith -Wno-pointer-sign -Warray-bounds -Wcast-align -Wformat-security -Wold-style-definition -D_REENTRANT -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/directfb -I/usr/include/libpng12 -I/usr/include/pixman-1 -MT libwireshark_la-emem.lo -MD -MP -MF .deps/libwireshark_la-emem.Tpo -c emem.c -fPIC -DPIC -o .libs/libwireshark_la-emem.o emem.c:153: error: redefinition of typedef ‘emem_header_t’ ftypes/../emem.h:37: error: previous declaration of ‘emem_header_t’ was here make[3]: *** [libwireshark_la-emem.lo] Error 1 make[3]: Leaving directory `/home/pascal/soft/wireshark/trunk/epan' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/home/pascal/soft/wireshark/trunk/epan' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/pascal/soft/wireshark/trunk' make: *** [all] Error 2 I do not have the time to look at it right now, so if you have some spare time feel free :) I believe Gerald already caught this in revision 45396 https://anonsvn.wireshark.org/viewvc?view=revisionrevision=45396 Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Bug 7775] Wireshark leaks memory when selecting packets
On Mon, Oct 8, 2012 at 2:58 PM, bugzilla-dae...@wireshark.org wrote: https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7775 --- Comment #10 from Anatoly aries@gmail.com 2012-10-08 14:58:26 EDT --- I've tried r45395, nothin is changed except wireshark has crashed:) But I got one more probably interesting fact: r45380 (w/o your fixes) on Vista 32 bit - memory consumption increases slightly and sometimes it decreases, but the same revision of XP 32bit - it always increases by ~2MB. This appears to be an XP-specific bug if anyone still has an XP install to test with (I don't). Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Bug 7775] Wireshark leaks memory when selecting packets
On Mon, Oct 8, 2012 at 3:31 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: Hi Evan, On Mon, Oct 08, 2012 at 03:06:29PM -0400, Evan Huus wrote: On Mon, Oct 8, 2012 at 2:58 PM, bugzilla-dae...@wireshark.org wrote: https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7775 --- Comment #10 from Anatoly aries@gmail.com 2012-10-08 14:58:26 EDT --- I've tried r45395, nothin is changed except wireshark has crashed:) But I got one more probably interesting fact: r45380 (w/o your fixes) on Vista 32 bit - memory consumption increases slightly and sometimes it decreases, but the same revision of XP 32bit - it always increases by ~2MB. This appears to be an XP-specific bug if anyone still has an XP install to test with (I don't). It mighe be OOM, for every packet we mmap 10MiB chunk of memory. Which is *not* unmaped in emem_free_all() nor ep_free_pool(). (AFAIR for performance reasons) Generally it'd be best to save somewhere mem-free_list before 1333 g_free(mem); If it's hard to do than emem_destroy_chunk() is your friend. [PS: I haven't test it yet, but looking at diff it makes sense, if not please move on ;)] I think you're right - I did all of my testing of the emem changes under the valgrind-wireshark.sh script, which disables chunks (and thus the mmap) so I never noticed it before. I have a patch (attached) which I believe ought to correctly share the free_list between the various pools, but for some reason I can't seem to track down it's causing random canary errors. Any ideas? Thanks, Evan share-free-list.patch Description: Binary data ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Bug 7775] Wireshark leaks memory when selecting packets
On Mon, Oct 8, 2012 at 5:17 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Mon, Oct 08, 2012 at 04:29:15PM -0400, Evan Huus wrote: On Mon, Oct 8, 2012 at 3:31 PM, Jakub Zawadzki darkjames...@darkjames.pl wrote: Hi Evan, On Mon, Oct 08, 2012 at 03:06:29PM -0400, Evan Huus wrote: On Mon, Oct 8, 2012 at 2:58 PM, bugzilla-dae...@wireshark.org wrote: https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7775 --- Comment #10 from Anatoly aries@gmail.com 2012-10-08 14:58:26 EDT --- I've tried r45395, nothin is changed except wireshark has crashed:) But I got one more probably interesting fact: r45380 (w/o your fixes) on Vista 32 bit - memory consumption increases slightly and sometimes it decreases, but the same revision of XP 32bit - it always increases by ~2MB. This appears to be an XP-specific bug if anyone still has an XP install to test with (I don't). It mighe be OOM, for every packet we mmap 10MiB chunk of memory. Which is *not* unmaped in emem_free_all() nor ep_free_pool(). (AFAIR for performance reasons) Generally it'd be best to save somewhere mem-free_list before 1333 g_free(mem); If it's hard to do than emem_destroy_chunk() is your friend. [PS: I haven't test it yet, but looking at diff it makes sense, if not please move on ;)] I think you're right - I did all of my testing of the emem changes under the valgrind-wireshark.sh script, which disables chunks (and thus the mmap) so I never noticed it before. I have a patch (attached) which I believe ought to correctly share the free_list between the various pools, but for some reason I can't seem to track down it's causing random canary errors. Any ideas? It works with your patch + attached patch. It doesn't crash, yes, but it leaks again. I've added emem_destroy_chunk() for now in revision 45412, until I can spend some time figuring out the correct logic for sharing freed blocks. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] buildbot failure in Wireshark (development) on Ubuntu-12.04-x64
This was caused by my removing the qm files in revision 45351. I believe the buildbot needs to be calling `lrelease QtShark.pro` to generate them before it calls `qmake QtShark.pro`? Evan On Sat, Oct 6, 2012 at 4:52 PM, buildbot-no-re...@wireshark.org wrote: The Buildbot has detected a new failure on builder Ubuntu-12.04-x64 while building Wireshark (development). Full details are available at: http://buildbot.wireshark.org/trunk/builders/Ubuntu-12.04-x64/builds/2091 Buildbot URL: http://buildbot.wireshark.org/trunk/ Buildslave for this Build: ubuntu-12.04-x64 Build Reason: scheduler Build Source Stamp: 45353 Blamelist: eapache BUILD FAILED: failed qt make sincerely, -The Buildbot ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] order of include files
On Thu, Oct 4, 2012 at 4:43 PM, Martin Kaiser li...@kaiser.cx wrote: Hi, should the order in which we include files make any difference? #include epan/packet.h #include epan/expert.h - ok #include epan/expert.h #include epan/packet.h - failure expert.h needs packet_info.h, which is included by packet.h Trivial fix is of course to include packet_info.h in experts.h. Are such issues worth fixing? Yes, fix them. It saves people unaware of the dependency countless headaches, and costs basically nothing if everything is properly guarded with #ifdefs. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c
On Sun, Sep 30, 2012 at 8:09 PM, Maynard, Chris christopher.mayn...@gtech.com wrote: Should similar changes be made here as well? capture_sync.c:1948:if (GetExitCodeProcess((HANDLE) capture_opts-fork_child, childstatus) tshark.c:1962:result1 = GetExitCodeProcess((HANDLE)*(pipe_input_p-child_process), - Chris The tshark code appears to be a copy of the one in gui_utils - I haven't looked too closely, but perhaps they should be deduplicated? The capture_sync code is interesting, since it treats the failure of GetExitCodeProcess as effectively an ignorable error. I don't know if that's indicative of best practice or it's simply another case of not knowing quite what to do with the return status. I don't have a problem with converting all three places for consistency, but without a Windows dev environment all I can really do is speculate. Evan Note to self: dig out old XP virtual machine and set it up for wireshark development ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c
On Sat, Sep 29, 2012 at 10:13 PM, Maynard, Chris
christopher.mayn...@gtech.com wrote:
Apologies, I meant to write:
if (!result || avail 0 || !result1 || childstatus != STILL_ACTIVE) {
I think I am too accustomed to *nix return values where 0 typically means
success, and so I read the documentation too quickly thinking the same
applied here.
... but since I'm not entirely sure what is needed here, I'll leave it to you
(or someone else) to make an appropriate change. I just wanted the builds to
succeed again so I could continue with some other stuff I was working on.
And don't worry about the buildbot failing; it happens to us all, even to the
best among us (and no, that would not be me in that category).
I don't have a Windows machine to test on, but I've attached a patch
which I *think* does the right thing. I made it into a g_error instead
of a g_warning, since I expect that having the call fail is
sufficiently unusual to warrant us bailing out ASAP, but that's more
of a gut call than anything.
If someone with a Windows machine can confirm that it works (at least
as far as not breaking capture on windows machines) then please feel
free to commit it.
Evan
GetExitCodeProcess.patch
Description: Binary data
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] [Wireshark-commits] rev 45212: /trunk/ui/gtk/ /trunk/ui/gtk/: gui_utils.c
On Sat, Sep 29, 2012 at 2:20 PM, Maynard, Chris
christopher.mayn...@gtech.com wrote:
Good point Bill. I hadn't actually looked too deeply here; I just wanted to
appease the buildbot. See:
http://buildbot.wireshark.org/trunk/builders/Windows-XP-x86/builds/2646/steps/nmake%20all/logs/stdio
So maybe a minimal patch such as this is what is needed? Maybe that was the
original intent?
- Chris
Index: ui/gtk/gui_utils.c
===
--- ui/gtk/gui_utils.c (revision 45212)
+++ ui/gtk/gui_utils.c (working copy)
@@ -694,7 +694,7 @@
{
HANDLEhandle;
DWORD avail = 0;
-gboolean result;
+gboolean result, result1;
DWORD childstatus;
pipe_input_t *pipe_input = data;
gint iterations = 0;
@@ -710,12 +710,13 @@
result = PeekNamedPipe(handle, NULL, 0, NULL, avail, NULL);
/* Get the child process exit status */
- GetExitCodeProcess((HANDLE)*(pipe_input-child_process),
childstatus);
+result1 = GetExitCodeProcess((HANDLE)*(pipe_input-child_process),
+ childstatus);
/* If the Peek returned an error, or there are bytes to be read
or the childwatcher thread has terminated then call the normal
callback */
-if (!result || avail 0 || childstatus != STILL_ACTIVE) {
+if (!result || avail 0 || result1 || childstatus != STILL_ACTIVE) {
/*g_log(NULL, G_LOG_LEVEL_DEBUG, pipe_timer_cb: data avail);*/
- Chris
Also not a Windows programmer, but this doesn't make sense to me. In
the case that the child process is still running and sane, but doesn't
have any packets then I expect GetExitCodeProcess to return TRUE and
set childstatus to STILL_ACTIVE, in which case there's no reason for
us to do the callback (which I believe we would do given this patch).
My understanding is that we should be g_warning if GetExitCodeProcess
returns FALSE, as that seems to indicate an error internal to win32?
In which case it should probably get its very own if statement before
the existing if(!result...
Evan
P.S. Apologies for the original build-breakage. At least something
interesting seems to have come of it.
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] Compilation failure - privileges.c: In function 'relinquish_special_privs_perm' - ignoring return value of 'setresgid', declared with attribute warn_unused_result [-Werror=unused-r
On Sat, Sep 29, 2012 at 4:05 PM, Bill Meier wme...@newsguy.com wrote:
On 8/21/2012 2:19 AM, Guy Harris wrote:
On Aug 20, 2012, at 12:49 PM, Kaul wrote:
If it were git, I'm sure I could easily use 'git bisect' and find the
issue.
As Evan Huus indicated, what you probably need to bisect is compiler
updates. :-)
The offending routine is probably relinquish_special_privs_perm()
(which has failed to check the return value of setresuid() and
setresgid() since at least 2007), and, if those calls were to fail, it's
not clear what Wireshark should do, so it's not clear that
relinquish_special_privs_perm() should return a success/failure
indication, and it's not clear that there's anything to do if any of the
set.*id routines return anything other than 0, so perhaps the right fix
is to cast the return values to void, so as to tell the compiler yes,
I'm deliberately ignoring the return value, because there's not much to
do if they fail.
I suppose we *could* have relinquish_special_privs_perm() return a
success/failure indication, and have its *callers* cheerfully ignore
the return value, or print a warning message/pop up a warning dialog
if they fail (if they fail, then we were started with special
privileges, but those privileges weren't sufficient to relinquish
them, in which case the developers should be notified so we can
figure out whether there's any way to relinquish them in that
situation).
I've started getting this warning also (F17...) (maybe just now since today
is the first time I've done a 'make clean' etc in a while).
So:
1. There's a long thread (which started Jul 24,2012) about
[PATCH] Declare set*id with warn_unused_result
at
http://old.nabble.com/-PATCH--Declare-set*id-with-warn_unused_result-td34204904.html
2. Doing a (void)... as suggested by Guy above doesn't prevent the error.
Using a hack like 'if(!setresgid() {}' does work but seems ugly;
returning a success/failure indication to the callers of
relinquish_special_privs_perm() seems like too much work.
My inclination: test the return value of the various set*id calls and if
fail, do g_error().
Thoughts ?
I'd be tempted to make it a g_warning() since Wireshark will
*probably* keep working despite the problem. The general approach
seems sound though.
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] filtered out messages show up briefly during live capture
On Tue, Sep 25, 2012 at 4:26 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: Hi folks, I'm doing a lot of live capturing these days (quite unusual for me). I have a (customized) 1.8.2 build on Linux and I've been noticing that while doing a live capture with a display filter I can see the packet list jitter sometimes (scroll down like there are more packets and then scroll back up) and indeed sometimes I have caught the packet list showing me packets that should not pass the dfilter only to have them disappear again off the packet list a few hundred msec later. Anyone else ever noticed that or have any ideas about it? I'm in no position to track it down at the moment; at the very least hopefully this email will remind me about it later... Regards, -Jeff ps. this may be somewhat of an unfair test because my X connection to the GUI isn't the fastest so that may make the delays more perceptible. I've never noticed anything like this before, but if I recall correctly Jakub has been doing some work on more efficiently updating the packet display - it's possible that he's already fixed this in trunk? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Problems Installing wireshark-1.8.2
Hi Saso, You probably need to install the development headers for some of those libraries. The package gtk2 only contains the runtime libraries, you need the gtk2-devel package with the source headers to build against it. Try: yum install gtk2-devel gtk3-devel qt-devel (I'm not on a redhat/fedora system at the moment, so no guarantees that those are the correct package names, but I believe they are) Cheers, Evan On Mon, Sep 24, 2012 at 9:03 AM, Saso dj.s...@gmail.com wrote: Linux level intermediate Linux version: 3.5.3-1.fc17.x86_64 (Fedora 17) Attached is config.log but fails at: checking for GTK+ - version = 2.12.0 and 3.0... no *** Could not run GTK+ test program, checking why... *** The test program failed to compile or link. See the file config.log for the *** exact error that occured. This usually means GTK+ is incorrectly installed. configure: error: Neither Qt nor GTK+ 2.12 or later are available, so Wireshark can't be compiled More system info: yum list gtk+ Loaded plugins: langpacks, presto, refresh-packagekit Installed Packages gtk+.x86_641:1.2.10-72.fc17 @fedora Available Packages gtk+.i686 1:1.2.10-72.fc17 fedora [root@Saso-T410 ~]# yum install Qt Loaded plugins: langpacks, presto, refresh-packagekit No package Qt available. * Maybe you meant: qt Error: Nothing to do [root@Saso-T410 ~]# yum install qt Loaded plugins: langpacks, presto, refresh-packagekit Package 1:qt-4.8.2-4.fc17.x86_64 already installed and latest version Nothing to do [root@Saso-T410 ~]# yum install gtk2 Loaded plugins: langpacks, presto, refresh-packagekit Package gtk2-2.24.11-1.fc17.x86_64 already installed and latest version Nothing to do [root@Saso-T410 ~]# yum install gtk3 Loaded plugins: langpacks, presto, refresh-packagekit Package gtk3-3.4.4-1.fc17.x86_64 already installed and latest version Nothing to do -- Dj Saso Ya #1 Spinna Las Vegas, NV ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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 wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Cmake build failing
CMake Error at CMakeLists.txt:59 (string): string sub-command REGEX, mode REPLACE needs at least 6 arguments total to command. And similar errors for lines 64 and 69. Not sure what's going on here - the calls conform to the convention specified by all the CMake docs I've found, and none of the build-bots are giving errors? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Accessing Wireshark SVN using Bazaar
This topic came up off-list and someone asked for a brief cheat-sheet / tutorial / explanation of what I currently use, so here it is. This email is more about the various version-control systems (SVN, Git, etc.) that manage Wireshark's source than it is about Wireshark itself, so if you're perfectly happy with the existing subversion setup you can stop reading here. -- First a bit of background for those who are new to the topic. Wireshark's source is currently managed using Subversion (SVN), and has been ever since the migration from CVS long before I knew what Wireshark even was. The current subversion tree can be browsed at [1]. Subversion is a mature, widely-respected tool. In the years since SVN was designed and originally developed, a new model of version-control system has been invented, known as the Distributed Version Control System. This model is typified by Git and Mercurial, two popular open-source DVCS tools. The DVCS model provides several advantages which are well-enumerated on the relevant Wikipedia article [2]. For just under a year now, Wireshark's code has also been available in a read-only Git repository, viewable at [3]. I have been a big fan of DVCS tools for a while now, and so for some time I would do my work in a read-only Git version of the source and then copy my changes over to SVN in order to commit. Obviously, that wasn't ideal. There is a program (git plugin?) called git-svn that in theory allows you to access a remote SVN repository as if it were a Git repository. I've never been able to get it to work, but if somebody has steps to follow for Wireshark, I'd be glad to hear them. There is another DVCS program (less popular than Git or Mercurial, but gaining ground now thanks to its use in Ubuntu) called Bazaar [4]. It also has a plugin (called bzr-svn [5]) for transparently accessing SVN repositories which I found quite easy to use. Both the Bazaar program and the bzr-svn plugin are available in the Ubuntu repositories, and are almost certainly in Debian/Fedora/etc. as well, though I haven't checked. Windows and Mac users can get it at [6]. Once installed, you can get the wireshark source as follows: $ bzr init-repo wireshark $ cd wireshark $ bzr checkout svn+http://anonsvn.wireshark.org/wireshark/trunk/ WARNING: the last command will take a *long* time (~24 hours I think, although I haven't formally timed it), as it has to fetch each SVN revision separately to build the local history. Once complete, however, normal operations are just as fast (if not faster) than they are in SVN. NOTE: Devs with commit access should use the normal writable url, just prepend svn+ as I did for the read-only url. With that done, you now have a working checkout in wireshark/trunk. You can treat it just like an SVN copy except you replace 'svn' commands with 'bzr' commands, so the majority of your time will be spent doing 'bzr update', 'bzr commit', 'bzr status', 'bzr diff', etc.. With a checkout like this, bzr treats the remote server just like SVN does - there's no need to worry about the complications of pushing and pulling and merging any more than normal. To convert it to a more Git-style checkout (where the main operations are push/pull/commit/merge instead of just update/commit) simply run 'bzr unbind' (to convert back to an SVN-style checkout, just run 'bzr bind'). I won't go into the details of using bazaar's advanced DCVS features here (it's very similar to Git and Mercurial, and the documentation is quite good), but this should be enough to kickstart anyone who is interested. I will of course do my best to answer questions anyone might have, but no guarantees. Wow, this email ended up being a bit longer than I'd anticipated :) Cheers, Evan [1] https://anonsvn.wireshark.org/viewvc/ [2] https://en.wikipedia.org/wiki/Distributed_revision_control [3] http://code.wireshark.org/git/wireshark [4] http://bazaar.canonical.com/en/ [5] http://doc.bazaar.canonical.com/beta/en/user-guide/svn_plugin.html [6] http://wiki.bazaar.canonical.com/Download ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Use of variadic macros
Variadic macros are forbidden in doc/README.developer since apparently not all compilers support them. However, I just stumbled across a variadic macro that has apparently been part of the regular build since around 2009 (packet-dcerpc-netlogon.c:65). Since nobody (to my knowledge) has complained about a failing build in that time period, I have to assume that whatever ancient compiler didn't support them is no longer being used. Unless anyone objects, I will remove that restriction from README.developer. Evan P.S. Are there any other C99 features that we forbid that are actually supported by supposedly C89-only compilers like MSVC? ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Use of variadic macros
On Sun, Sep 23, 2012 at 6:10 PM, Guy Harris g...@alum.mit.edu wrote: On Sep 23, 2012, at 1:59 PM, Evan Huus eapa...@gmail.com wrote: Variadic macros are forbidden in doc/README.developer since apparently not all compilers support them. However, I just stumbled across a variadic macro that has apparently been part of the regular build since around 2009 (packet-dcerpc-netlogon.c:65). Since nobody (to my knowledge) has complained about a failing build in that time period, I have to assume that whatever ancient compiler didn't support them is no longer being used. They're supported with Microsoft's compilers at least as far back as Visual Studio 2005: http://msdn.microsoft.com/en-us/library/ms177415(v=vs.80).aspx There's no earlier version of the documentation offered by the Other Versions dropdown, but I don't know whether that means earlier compilers, such as MSVC 6, didn't support them or we don't have the MSVC 6 documentation online. This page: http://bytes.com/topic/c/answers/220087-macros-variable-parameter-list-ms-vc-compiler-6-0-a has a claim that MSVC 6 didn't support variadic macros. If that's the case, I don't personally have a problem with kicking MSVC 6 to the curb; we may already have done so in the documentation, as http://www.wireshark.org/docs/wsdg_html_chunked/ChToolsMSChain.html only mentions it when it says The official Wireshark 1.8.x releases are compiled using Microsoft Visual C++ 2010 SP1. The official 1.2, 1.4, and 1.6 releases are and were compiled using Microsoft Visual C++ 2008 SP1. Other past releases, including the 1.0 branch, were compiled using Microsoft Visual C++ 6.0., and doesn't at all discuss using MSVC++ 6. I don't know what other compilers, such as Sun C^WOracle Solaris Studio, HP's ANSI C compiler, or IBM's XL C support. My *guess* is that older versions of those compilers may not have supported them but that they've all up-to-date with C99 features. Sun C fully supported C99 as of Sun Studio 12 (not to be confused with Sun Studio, Memphis :-)): http://docs.oracle.com/cd/E19205-01/820-4155/c.html#about *if* you specify -xc99; I don't know whether it or later versions support variadic macros without -xc99 or whether earlier versions supported them. IBM C for AIX fully supported C99 as of V6.0 in 2002: http://www-01.ibm.com/common/ssi/cgi-bin/ssialias?infotype=ansubtype=casupplier=897appname=IBMLinkRedirectletternum=ENUS202-161 and XL C for AIX http://www-01.ibm.com/software/awdtools/xlcpp/aix/features/ supports C99 now (with no indication in the document of when it started, but the Wikipedia page for C99, whence I got these references, says it started with V11.1). I don't know whether either of them have separate C89 and C99 modes (which they might, for the benefit of those developing code that needs to build on C89 compilers) or whether, in C89 mode, they support extensions such as variadic macros. Given that one of IBM's compilers is the reason why we ban // comments - it disallows them by default - I suspect that, when not running in C99 (or C11?) mode, they do not support them. The HP ANSI C compiler supports C99 at least as of A.06.25 http://h21007.www2.hp.com/portal/download/files/unprot/aCxx/Online_Help/getstarted.htm I don't know whether it has separate C89 and C99 modes (which they might, for the benefit of those developing code that needs to build on C89 compilers) or whether, in C89 mode, it supports extensions such as variadic macros. P.S. Are there any other C99 features that we forbid that are actually supported by supposedly C89-only compilers like MSVC? // comments are, I think, supported by MSVC; I don't know whether any UN*X C compilers support them without being told to run in C99 or C11 (or GCC compatible?) mode. Turning on full C99 mode for compilers does run the risk of people using C99 features *not* supported by MSVC++ and not having them discovered until the build breaks. I'll leave the README unchanged for the moment then. Just out of curiosity, has anyone ever tried compiling on Windows with something like MinGW or another compiler? I expect there would be some initial pain, but if it allows us to drop MSVC and properly move to C99, I think would be an overall win. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 45019: /trunk/ /trunk/tools/lemon/: CMakeLists.txt
On Thu, Sep 20, 2012 at 5:02 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: Joerg Mayer wrote: On Thu, Sep 20, 2012 at 02:55:22AM +, eapa...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=45019 User: eapache Date: 2012/09/19 07:55 PM Log: Fix lemon build with cmake by defining _U_. I feel like there's a better way than all the horrid escaping I had to do, but I don't know what it is. Directory: /trunk/tools/lemon/ ChangesPath Action +6 -0 CMakeLists.txtModified Well, you could include config.h again, which defined _U_ before it was removed. Ah, so cmake was putting _U_ in config.h? I remember wondering once a long time ago why _U_ wasn't in config.h (in autofoo and nmake); any reason not to (uniformly) put it there? Not that I know of. Feel free to revert 45019 if there is a better solution. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 44860: /trunk/ /trunk/asn1/camel/: camel.cnf /trunk/asn1/charging_ase/: packet-charging_ase-template.c /trunk/asn1/cmp/: packet-cmp-template.c /trunk/asn1/c
On Tue, Sep 18, 2012 at 3:35 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: darkja...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=44860 User: darkjames Date: 2012/09/10 02:40 PM Log: Initial commit to support yet another method of passing data between dissectors. Add new parameter 'data' to heur_dissector_t and new_dissector_t, for now it's always NULL Does anybody have an idea of how to handle this in custom dissectors which need to be compiled in both 1.8 and trunk (without branching said dissectors--something I really would like to avoid)? I'm contemplating putting a macro in packet.h like DISSECTORS_HAVE_DATA_PARAM and adding the appropriate #ifdef/#else logic in my custom dissectors. Any objections? Any better ideas? I filed something similar as a bug long before I was a core dev: https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=6134 I still don't know enough about the build system to feel comfortable fixing it myself once and for all (especially given the mix of autotools, cmake, nmake) but I still think that a general version macro would be a much better way than manually adding a macro for every API change. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Disabling a dissector doesn't seem to quite work.
On Sat, Sep 15, 2012 at 2:06 PM, Christopher Maynard christopher.mayn...@gtech.com wrote: Evan Huus eapache@... writes: There is already a (commented-out) function called dissector_add_uint_sanity_check which does warn on duplicate port registrations and on registrations to port 0. It produces 157 warnings when enabled in the default build. I don't know how many duplicate string registrations there are, but there are definitely some based on valgrind's output. I took a quick look at EIGRP since tshark was reporting these strange warnings against itself: ** (tshark.exe:6984): WARNING **: ip.proto: eigrp registering using pattern 88 already registered by eigrp ** (tshark.exe:6984): WARNING **: ddp.type: eigrp registering using pattern 88 already registered by eigrp ** (tshark.exe:6984): WARNING **: ipx.socket: eigrp registering using pattern 34238 already registered by eigrp Hmm, so it seems that the doxygen comments are somehow causing this? For example, the following patch eliminates the warnings for me: Index: epan/dissectors/packet-eigrp.c === --- epan/dissectors/packet-eigrp.c (revision 44888) +++ epan/dissectors/packet-eigrp.c (working copy) @@ -3351,7 +3351,6 @@ } /** - *@fn void proto_reg_handoff_eigrp(void) * * @param[in] void * @param[out] None Oh boy, this isn't nice. The scripts /tools/make-dissector-reg[.py] use regexes to look for definitions that look like protocol registration functions. The scripts add any functions they find to an array that is called on startup to register dissectors. Because the doxygen comments have the exact form of the function definition, the regex matches twice (once on the doxygen comment and once on the real definition) and proto_reg_handoff_eigrp() gets added to the array twice, meaning it is called twice on startup, which is a problem for all sorts of reasons. The obvious solution for now is to remove the comments that are getting falsely picked up as function definitions, but the better fix is to the make-dissector-reg scripts. Is it valid for there to be two register functions in a file, or could the scripts simply limit to one entry per file? Or someone who groks the regexes could fix them to ignore lines that are commented out... ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Disabling a dissector doesn't seem to quite work.
On Fri, Sep 7, 2012 at 6:09 PM, Evan Huus eapa...@gmail.com wrote: On Fri, Sep 7, 2012 at 5:47 PM, Maynard, Chris christopher.mayn...@gtech.com wrote: Recently another old proprietary protocol (I’ll call it FOO) was brought to my attention, and I was asked to write a dissector for it. In doing so, I discovered a conflict with another dissector, namely SNA. Initially I thought that simply disabling SNA when analyzing FOO would be good enough for our purposes; however, even after disabling SNA, the FOO dissector was never called. The conflict arises because both dissectors, SNA and FOO, register as follows: dissector_add_uint(llc.dsap, SAP_SNA2, [sna|foo]_handle); I found that even with SNA disabled, I had to comment out the above line of code from the packet-sna.c source file before LLC would successfully hand off dissection to FOO. To illustrate this, I’ve attached a very stripped down and slightly modified version of packet-foo.c, along with a simple foo24.pcap file in case anyone would care to take a look at it. I have since found an alternate solution, but I was surprised that disabling a protocol does not seem to have the completely desired effect. While the packet does not get dissected as SNA, other dissectors are apparently not given the opportunity to dissect the packet even when the SNA dissector is disabled. But beyond LLC and SNA, I was thinking/wondering that maybe this is a general problem affecting all dissectors and that some general solution might be needed? A couple of problems here: - When two protocols register with the same uint value in a table, the second one just overwrites the first. - When two protocols register with the same uint value in a table, we leak memory. It's visible in valgrind, as there are already a couple in the default build. Just run libtool --mode=execute valgrind --leak-check=full ./tshark and grep for dissector_add_uint. - Even disabled protocols are registered, and thus are added to the appropriate table. This can overwrite enabled protocols, depending on registration order. I've got somewhere to be right now, but I'll try and do further analysis this weekend. There is already a (commented-out) function called dissector_add_uint_sanity_check which does warn on duplicate port registrations and on registrations to port 0. It produces 157 warnings when enabled in the default build. I don't know how many duplicate string registrations there are, but there are definitely some based on valgrind's output. I have fixed the memory leaks (about 2K by default) in revision 44814. This doesn't stop us from overwriting existing registrations, it just prevents us from leaking memory when we do overwrite existing registrations. The fundamental problem is that registration happens once right when the program starts, while protocols can be enabled/disabled arbitrarily while an instance is running. Triggering a reregistration on a protocol enable/disable would be painful, and I don't even know if it would solve the problem. A possible solution would be to store a linked list of dtbl_entries for each registration instead of the single entry we currently store. Then dissector_try_uint() can iterate through and try all of the dtbl_entries whose associated protocols are marked as enabled. This might lead to odd behaviour when multiple protocols are enabled for the same port though... we have no way of knowing which one to pick, so we'd have to be careful to avoid bad things (multiple protocols dissecting the same tvb subset, or worse?). New-style dissectors that do heuristics first might make that manageable though. Thoughts? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Disabling a dissector doesn't seem to quite work.
On Fri, Sep 7, 2012 at 5:47 PM, Maynard, Chris christopher.mayn...@gtech.com wrote: Recently another old proprietary protocol (I’ll call it FOO) was brought to my attention, and I was asked to write a dissector for it. In doing so, I discovered a conflict with another dissector, namely SNA. Initially I thought that simply disabling SNA when analyzing FOO would be good enough for our purposes; however, even after disabling SNA, the FOO dissector was never called. The conflict arises because both dissectors, SNA and FOO, register as follows: dissector_add_uint(llc.dsap, SAP_SNA2, [sna|foo]_handle); I found that even with SNA disabled, I had to comment out the above line of code from the packet-sna.c source file before LLC would successfully hand off dissection to FOO. To illustrate this, I’ve attached a very stripped down and slightly modified version of packet-foo.c, along with a simple foo24.pcap file in case anyone would care to take a look at it. I have since found an alternate solution, but I was surprised that disabling a protocol does not seem to have the completely desired effect. While the packet does not get dissected as SNA, other dissectors are apparently not given the opportunity to dissect the packet even when the SNA dissector is disabled. But beyond LLC and SNA, I was thinking/wondering that maybe this is a general problem affecting all dissectors and that some general solution might be needed? A couple of problems here: - When two protocols register with the same uint value in a table, the second one just overwrites the first. - When two protocols register with the same uint value in a table, we leak memory. It's visible in valgrind, as there are already a couple in the default build. Just run libtool --mode=execute valgrind --leak-check=full ./tshark and grep for dissector_add_uint. - Even disabled protocols are registered, and thus are added to the appropriate table. This can overwrite enabled protocols, depending on registration order. I've got somewhere to be right now, but I'll try and do further analysis this weekend. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Feature Request: Sync Capture Time to NTP packets
On Wed, Sep 5, 2012 at 12:54 AM, Jeff Klingler jkling...@gmail.com wrote: Hi, Kudos to all you developers for such a great tool! I did a brief search and couldn't find any discussion of a feature I would like to see added: Sync the capture time in a file relative to the time settings found in an NTP server responses. Maybe select a particular server, or a particular NTP response packet, right click and say Time Shift. This would be magical for those packet captures where you don't control the system clock. Hi Jeff, Thanks for the idea, it does sound useful in certain situations. I would appreciated it if you could file an enhancement request for it on bugzilla [1], as emails on the mailing-list tend to be forgotten about after a while. That's not to say that someone is guaranteed to look at it if it's on bugzilla - we're mostly volunteers after all - but it stands a much better chance. And of course we always accept patches if you want to code it yourself :) Cheers, Evan [1] https://bugs.wireshark.org/bugzilla/ ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Removing string constants from assertions
Hi all, I've noticed in several places the pattern of adding a message to assertions in the form of a string constant: g_assert(condition explanation); This seems dangerous to me, primarily because if anyone ever mistypes the as a || then the assertion becomes dead code - it will always pass. Also (though less important): - it doesn't really add any benefit over simply putting the message in a comment - a developer will have to open up the code anyways to figure out what the problem is - it bloats the size of the binary with a couple of unnecessary string constants Barring any objections I will convert all such assertion messages to regular comments. Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Removing string constants from assertions
On Tue, Sep 4, 2012 at 4:53 PM, Guy Harris g...@alum.mit.edu wrote:
On Sep 4, 2012, at 1:39 PM, Evan Huus wrote:
I've noticed in several places the pattern of adding a message to
assertions in the form of a string constant:
g_assert(condition explanation);
This seems dangerous to me, primarily because if anyone ever mistypes
the as a || then the assertion becomes dead code - it will always
pass.
Yes, that's risky.
Also (though less important):
- it doesn't really add any benefit over simply putting the message in
a comment - a developer will have to open up the code anyways to
figure out what the problem is
I'm not *entirely* sure that's the case; the message printed with the version
of GLib on my machine (2.32.3) includes the entire assertion string:
ERROR:sourcefile.c:{lineno}:{func}: assertion failed: ({test
condition} {string})
It Might Be Nice if there were a version of g_assert() that took two
arguments - a condition and a description string - and printed
ERROR:sourcefile.c:{lineno}:{func}: assertion failed: {test
condition} : {description})
or something such as that, but there isn't.
We might be able to hack up a macro of that sort (with a helper routine, just
as the existing g_assert macros have g_assertion_message_ helper routines).
I mis-thought on that one. I meant to say that the developer will have
to open up the code anyways to *fix* the problem (and non-developers
probably don't care about the gory details behind an assertion
failure).
I agree that a macro would be nice, but I'm not sure how much of an
ugly hack it would end up being. I'm not too familiar with the bowels
of C macros though, so it could be easy.
I will convert the string constants to comments for now, and if
someone wants to write such a macro then they're free to grep through
for g_assert calls and fix them up.
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 3:14 AM, Anders Broman a.bro...@bredband.net wrote: Evan Huus skrev 2012-08-30 04:31: On Wed, Aug 29, 2012 at 7:28 PM, Anders Broman a.bro...@bredband.net wrote: Jeff Morriss skrev 2012-08-30 00:29: Evan Huus wrote: I'm not 100% convinced either way, but I have to admit I do like having all the dissectors in the same directory. make -j 40 (on my 32-vCPU SPARC) works better that way ;-). I'm pretty sure an autotools-generated Makefile will already recurse to fill the given job-count as long as there aren't any weird dependencies in place, so it shouldn't make any difference. Can't speak for cmake or windows builds. Only if all the files are in one Makefile (e.g., epan/dissectors/Makefile). If each subdir has its own Makefile then each directory is processed one at a time (in my experience). More seriously, I imagine I'd find it easier to do: vi epan/dissectors/packet-xmpptabtab instead of: vi epan/dissectors/packet-xmpptabtab[1]^H^H^H^H^H^H^H^H^H^H^Hxmpptab/tabtab [1] insert grumpy remark here Fair enough. So another tweak to the suggested naming: packet-xmpp/xmpp-whatever.c In fact taking your suggestion of removing packet- from all the file names would also achieve the same thing. I'm not particularly fond of the idea - just being conservative perhaps; but how many subdirectories are acceptable before it gets out of hand - 1000, one for every protocol in WS or a smaller number ;-) I expect most dissectors will stay in the root of epan/dissectors/. My understanding is that this would only be in a few select cases (bluetooth and xmpp are the ones that come to mind) where there is a clear logical grouping of a large number (16 for bluetooth, 14 for xmpp) of files. It makes sense to put them in their own folder. I just gave epan/dissectors/ a quick scan. Here are the other large groupings that immediately stand out: - aim (23 files) - dcerpc (111 files) - gsm (23 files) - h### (33 files) (is there a better generic name for this category?) - ipmi (12 files) - smb (15 files) - zbee (14 files) There are lots and lots of smaller groups of 4-5 files, but I wouldn't expect them to get their own folder. More input is always welcome :) h### comes from ITU (http://www.itu.int/en/ITU-T/publications/Pages/recs.aspx) ITU-T Series of Recommendations H: Audiovisual and multimedia systems I'm not sure we would like to have all of them in the same folder but that depends on the logic we want to apply, likewise some of those gsm protocol has nothing else in common than being used in Mobile system of GSM type (e.g not dissection for one protocol spread in several files) should UMTS and LTE have their own folders as well in that case? ANSI/CDMA? Perhaps it's more clear cut in aim dcerpc etc and makes more sense. I was just going by the existing file names, so if they're not actually logically related then they shouldn't be grouped. Being conservative on a first pass makes sense, and we can see how it works out for the really obvious cases. Unless there are any other objections I'd like to propose we start with these four as a sort of trial run: - aim - bluetooth - dcerpc - xmpp And with the naming scheme: packet-xmpp/xmpp-whatever.c Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 9:23 AM, Roland Knall rkn...@gmail.com wrote: Hi Would you like to enforce a value for the minimum number of subsequent files in the subdirectories? I would assume you'd need 5 or 6 files at least to make a folder worthwhile, but I don't think that's a hard rule. None of the groups proposed for the initial run have less than 14. As I wrote the opensafety package, I would like to split it up a little bit to make it more maintainable, as well as include two new subdissectors, which use the openSAFETY protocol, but are not necessarily part of it. For the subdissectors, it depends on how tightly they are bound to opensafety. If they could theoretically be carried on other protocols as well then they shouldn't be grouped with it, but if they are restricted to opensafety (in the sense that they use some special fields or features of opensafety and so actually couldn't be carried on, say, TCP, without changing the protocol), then they can logically be grouped with it. Again, probably not a hard rule, but a good guideline. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 9:45 AM, Graham Bloice graham.blo...@trihedral.com wrote: -Original Message- From: wireshark-dev-boun...@wireshark.org [mailto:wireshark-dev- boun...@wireshark.org] On Behalf Of Evan Huus Sent: 30 August 2012 14:31 To: Developer support list for Wireshark Subject: Re: [Wireshark-dev] RFD: Creating subdirectories in epan/dissectors/ On Thu, Aug 30, 2012 at 9:23 AM, Roland Knall rkn...@gmail.com wrote: Hi Would you like to enforce a value for the minimum number of subsequent files in the subdirectories? I would assume you'd need 5 or 6 files at least to make a folder worthwhile, but I don't think that's a hard rule. None of the groups proposed for the initial run have less than 14. As I wrote the opensafety package, I would like to split it up a little bit to make it more maintainable, as well as include two new subdissectors, which use the openSAFETY protocol, but are not necessarily part of it. For the subdissectors, it depends on how tightly they are bound to opensafety. If they could theoretically be carried on other protocols as well then they shouldn't be grouped with it, but if they are restricted to opensafety (in the sense that they use some special fields or features of opensafety and so actually couldn't be carried on, say, TCP, without changing the protocol), then they can logically be grouped with it. Again, probably not a hard rule, but a good guideline. [Graham Bloice said] Some folks have articulated the drawbacks (to them) of making these changes but I haven't seen any actual advantages listed. Can anyone list them as they see it? I think it boils down to better structure and scalability. We have an enormous number of files in epan/dissectors/ and I fully expect it to continue to grow. The flat structure is already unwieldy and it's only going to get worse. Logically grouping certain dissectors reduces the number of items at the epan/dissectors/ level. It also makes it much easier to work on a single cluster: if you're working on some enhancements to the bluetooth dissectors, it's much easier to work in epan/dissectors/packet-bluetooth/ than to try and wade through all the other hundreds of files in epan/dissectors/ every time you need to open another bluetooth-related file. It should also make the groupings easier for new developers to grok. It's pretty obvious that everything in packet-bluetooth/ must be bluetooth-related, even if the filename is something like packet-hci_h4.c (which is a real bluetooth dissector file, for those who didn't know). *I* know that hci stands for Host/Controller Interface which is a bluetooth-related term, but only because I looked it up for the purposes of this email, and I still don't know what the h4 is. New developers shouldn't have to do that kind of research (or peek at the file header) to get an idea of what a dissector does. Arguably my last point could be fixed by just using better names in epan/dissectors/, but then you end up with names that are uncomfortably long: packet-bluetooth_host-controller-interface_hsomethingsomething4.c anyone? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 1:46 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: Evan Huus wrote: On Thu, Aug 30, 2012 at 9:45 AM, Graham Bloice graham.blo...@trihedral.com wrote: -Original Message- From: wireshark-dev-boun...@wireshark.org [mailto:wireshark-dev- boun...@wireshark.org] On Behalf Of Evan Huus Sent: 30 August 2012 14:31 To: Developer support list for Wireshark Subject: Re: [Wireshark-dev] RFD: Creating subdirectories in epan/dissectors/ On Thu, Aug 30, 2012 at 9:23 AM, Roland Knall rkn...@gmail.com wrote: Hi Would you like to enforce a value for the minimum number of subsequent files in the subdirectories? I would assume you'd need 5 or 6 files at least to make a folder worthwhile, but I don't think that's a hard rule. None of the groups proposed for the initial run have less than 14. As I wrote the opensafety package, I would like to split it up a little bit to make it more maintainable, as well as include two new subdissectors, which use the openSAFETY protocol, but are not necessarily part of it. For the subdissectors, it depends on how tightly they are bound to opensafety. If they could theoretically be carried on other protocols as well then they shouldn't be grouped with it, but if they are restricted to opensafety (in the sense that they use some special fields or features of opensafety and so actually couldn't be carried on, say, TCP, without changing the protocol), then they can logically be grouped with it. Again, probably not a hard rule, but a good guideline. [Graham Bloice said] Some folks have articulated the drawbacks (to them) of making these changes but I haven't seen any actual advantages listed. Can anyone list them as they see it? I think it boils down to better structure and scalability. We have an enormous number of files in epan/dissectors/ and I fully expect it to continue to grow. The flat structure is already unwieldy and it's only going to get worse. Unwieldy how? Except for having to know not to do vi epan/dissectors/tabtab (for fear of too many pages of output) I don't find the directory unwieldy. That's part of it - I still do that accidentally on occasion. The other part of it is just that the number of files is overwhelming. It's not necessarily inefficient for the computer, but it is certainly inefficient for a mental model of the source structure, and it feels daunting, which is something we want to avoid in order to encourage new developers. I know when I checked out the source tree for the very first time I looked at the size of it and had very much a where do I start?! moment. I admit I don't use any kind of graphical interface to look in the directory... I imagine pointing a file explorer in there might be somewhat painful. It is. I tend not to browse the tree with a GUI, but I have in the past and it was a nightmare. Logically grouping certain dissectors reduces the number of items at the epan/dissectors/ level. It also makes it much easier to work on a single cluster: if you're working on some enhancements to the bluetooth dissectors, it's much easier to work in epan/dissectors/packet-bluetooth/ than to try and wade through all the other hundreds of files in epan/dissectors/ every time you need to open another bluetooth-related file. Don't you get the same thing by naming the files packet-high_level_protocol-subprotocol, etc.? To a point, but shell auto-complete is much more efficient if it's a sub-directory. It should also make the groupings easier for new developers to grok. It's pretty obvious that everything in packet-bluetooth/ must be bluetooth-related, even if the filename is something like packet-hci_h4.c (which is a real bluetooth dissector file, for those who didn't know). *I* know that hci stands for Host/Controller Interface which is a bluetooth-related term, but only because I looked it up for the purposes of this email, and I still don't know what the h4 is. New developers shouldn't have to do that kind of research (or peek at the file header) to get an idea of what a dissector does. Arguably my last point could be fixed by just using better names in epan/dissectors/, but then you end up with names that are uncomfortably long: packet-bluetooth_host-controller-interface_hsomethingsomething4.c anyone? Well, no, but how about packet-bthci_h4.c (to match the naming convention of the other bluetooth files)? That would certainly help, but again it's not immediately obvious to new devs that packet-bt*.c is bluetooth, and spelling out packet-bluetooth_*.c gives pretty long names. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 6:21 PM, Jaap Keuter jaap.keu...@xs4all.nl wrote: On 08/30/2012 04:31 AM, Evan Huus wrote: On Wed, Aug 29, 2012 at 7:28 PM, Anders Bromana.bro...@bredband.net wrote: Jeff Morriss skrev 2012-08-30 00:29: Evan Huus wrote: I'm not 100% convinced either way, but I have to admit I do like having all the dissectors in the same directory. make -j 40 (on my 32-vCPU SPARC) works better that way ;-). I'm pretty sure an autotools-generated Makefile will already recurse to fill the given job-count as long as there aren't any weird dependencies in place, so it shouldn't make any difference. Can't speak for cmake or windows builds. Only if all the files are in one Makefile (e.g., epan/dissectors/Makefile). If each subdir has its own Makefile then each directory is processed one at a time (in my experience). More seriously, I imagine I'd find it easier to do: vi epan/dissectors/packet-xmpptabtab instead of: vi epan/dissectors/packet-xmpptabtab[1]^H^H^H^H^H^H^H^H^H^H^Hxmpptab/tabtab [1] insert grumpy remark here Fair enough. So another tweak to the suggested naming: packet-xmpp/xmpp-whatever.c In fact taking your suggestion of removing packet- from all the file names would also achieve the same thing. I'm not particularly fond of the idea - just being conservative perhaps; but how many subdirectories are acceptable before it gets out of hand - 1000, one for every protocol in WS or a smaller number ;-) I expect most dissectors will stay in the root of epan/dissectors/. My understanding is that this would only be in a few select cases (bluetooth and xmpp are the ones that come to mind) where there is a clear logical grouping of a large number (16 for bluetooth, 14 for xmpp) of files. It makes sense to put them in their own folder. I just gave epan/dissectors/ a quick scan. Here are the other large groupings that immediately stand out: - aim (23 files) - dcerpc (111 files) - gsm (23 files) - h### (33 files) (is there a better generic name for this category?) - ipmi (12 files) - smb (15 files) - zbee (14 files) There are lots and lots of smaller groups of 4-5 files, but I wouldn't expect them to get their own folder. More input is always welcome :) Evan Oke, I'm still missing the quantitative rationale here. When I'm looking at the number of dissector files in trunk I get: jaap@phoenix:~/src/wireshark/trunk/epan/dissectors$ ls -1 | grep ^packet | wc -l 1501 Your list gives 23+111+23+33+12+15+14 = 231, that's a mere 15%. That doesn't really alleviate the problem. I don't think the intent in the original proposal was that this would magically reduce epan/dissectors/ to a fraction of its original size, but 15% is a non-trivial improvement. Also, my list was only a quick scan based on file-name similarities. I'm sure there are other logical groups that aren't as obvious simply because the file names are inconsistent. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Thu, Aug 30, 2012 at 8:29 PM, Ed Beroset bero...@mindspring.com wrote: Evan Huus wrote: On Thu, Aug 30, 2012 at 1:46 PM, Jeff Morriss jeff.morriss...@gmail.com wrote: Unwieldy how? Except for having to know not to do vi epan/dissectors/tabtab (for fear of too many pages of output) I don't find the directory unwieldy. That's part of it - I still do that accidentally on occasion. The other part of it is just that the number of files is overwhelming. It's not necessarily inefficient for the computer, but it is certainly inefficient for a mental model of the source structure, and it feels daunting, which is something we want to avoid in order to encourage new developers. I know when I checked out the source tree for the very first time I looked at the size of it and had very much a where do I start?! moment. I'm relatively new to Wireshark development (only a few years), so I remember that moment too. However, I'm not sure that organizing things into subdirectories would really help that much. What you've identified though, is a real thing that might be addressed, which is that regardless of how the files are physically organized, it would be useful to structure things to aid human beings who look at and work with the source files. Doxygen, which is already lightly supported, could help there without a lot of additional effort. I think that might be a better way to approach this problem than rearranging the source code directory structure. What do you think? It might not help a lot, but it will help a little. Doxygen is a good tool which we should probably be using more - perhaps that should be its own thread? - but its use doesn't exclude trying to find a logical way to structure the source. Also, I would hardly call this a rearrangement. All we're trying to do is make some existing structure a bit more explicit. ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
Having re-read the entire thread, I've gathered the following list of objections. I think it covers all of the concerns mentioned so far (in no particular order): 1 potential for file-name collisions 2 increased difficulty in using tab-completion 3 potential for less parallel make 4 more complicated to search dissector source 5 concern that the number of folders would scale as poorly as the number of files 6 concern that it wouldn't actually provide any benefit 1 and 2 are only issues with the original, rough draft of the naming scheme. The refined naming scheme proposed later in the thread does not have either issue. 3 is only an issue if we write the makefiles wrong. Now that somebody's mentioned it, hopefully we wouldn't make that mistake :) 4 is valid, but minor. I would hope that most developers use a tool like ctags, cscope, or similar to index the code. All of those tools will work regardless of how the source is layed out. Manual greps will be more complicated, but if you have to run a manual grep on a codebase the size of Wireshark's then I think there's already something wrong. 5 is reason to be conservative in our use of folders, but not reason to discard the idea. As long as we limit folders to groups with a large number of files (10+? more?) then they'll still provide a benefit. 1000 folders is way too many, but it's still better than 15000 files! 6 I'm not clear on, since the benefits seem so obvious to me. Perhaps I've just done a poor job of explaining. I simply feel that epan/dissectors/packet-bluetooth/ would be a good thing for the same reason that epan/dissectors/ is a good thing: it provides additional logical structure that turns a list (inefficient) into a tree (efficient). If people are more in favour of using patterned names like packet-bluetooth-* to denote that structure, then why do we use folders at all? As Jeff pointed out, we moved all the dissectors together into epan/dissectors/ for a reason. That reason still applies here. At this point I feel that I've said everything I can possibly say on the subject. If people are still generally against the idea then so be it, but I'm not going to argue the point anymore. All the best, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Wed, Aug 29, 2012 at 11:47 AM, Jeff Morriss jeff.morriss...@gmail.com wrote: Joerg Mayer wrote: What I'd like to do is put these dissectors that belong *to a single protocol* into a subdirectory of that name, i.e. move them to xmpp/packet-conference.c xmpp/packet-conference.h xmpp/packet-core.c xmpp/packet-core.h xmpp/packet-gtalk.c xmpp/packet-gtalk.h xmpp/packet-jingle.c xmpp/packet-jingle.h xmpp/packet-other.c xmpp/packet-other.h xmpp/packet-utils.c xmpp/packet-utils.h xmpp/packet-xmpp.c- special case, main protocol name xmpp/packet-xmpp.h- special case, main protocol name What do you think? I'm not 100% convinced either way, but I have to admit I do like having all the dissectors in the same directory. make -j 40 (on my 32-vCPU SPARC) works better that way ;-). I'm pretty sure an autotools-generated Makefile will already recurse to fill the given job-count as long as there aren't any weird dependencies in place, so it shouldn't make any difference. Can't speak for cmake or windows builds. More seriously, I imagine I'd find it easier to do: vi epan/dissectors/packet-xmpptabtab instead of: vi epan/dissectors/packet-xmpptabtab[1]^H^H^H^H^H^H^H^H^H^H^Hxmpptab/tabtab [1] insert grumpy remark here Fair enough. So another tweak to the suggested naming: packet-xmpp/xmpp-whatever.c That should avoid file-name conflicts and work well with existing muscle-memory. A good shell will even complete the /xmpp- in a single tab assuming there are no makefiles in the directory to screw things up. (That's another question though - do we keep one top-level makefile in epan/dissectors/, or do we add one in each sub-directory?) I also like being able to, for example: grep tvb_get_ptr epan/dissectors/packet-*.c instead of: grep tvb_get_ptr epan/dissectors/packet-*.c epan/dissectors/*/packet-*.c That's what tools like cscope [1] are for :) Alternatively, there is a nicer way with grep: grep -R --include=packet-*.c tvb_get_ptr epan/dissectors/ [1] http://cscope.sourceforge.net/ ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] rev 44694: /trunk/epan/crypt/ /trunk/epan/crypt/: airpdcap.c
On Wed, Aug 29, 2012 at 12:15 PM, cmayn...@wireshark.org wrote: http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=revrevision=44694 User: cmaynard Date: 2012/08/29 09:15 AM Log: Allow wpa-psk decryption keys to be successfully entered. Problems reported on ask.wireshark.org here: 1) http://ask.wireshark.org/questions/13951/invalid-key-format-wireshark-182 2) http://ask.wireshark.org/questions/13688/error-updating-record-invalid-key-format #BACKPORT (to 1.8) Directory: /trunk/epan/crypt/ ChangesPath Action +6 -6 airpdcap.cModified Also bug 7661? https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7661 ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] RFD: Creating subdirectories in epan/dissectors/
On Wed, Aug 29, 2012 at 7:28 PM, Anders Broman a.bro...@bredband.net wrote: Jeff Morriss skrev 2012-08-30 00:29: Evan Huus wrote: I'm not 100% convinced either way, but I have to admit I do like having all the dissectors in the same directory. make -j 40 (on my 32-vCPU SPARC) works better that way ;-). I'm pretty sure an autotools-generated Makefile will already recurse to fill the given job-count as long as there aren't any weird dependencies in place, so it shouldn't make any difference. Can't speak for cmake or windows builds. Only if all the files are in one Makefile (e.g., epan/dissectors/Makefile). If each subdir has its own Makefile then each directory is processed one at a time (in my experience). More seriously, I imagine I'd find it easier to do: vi epan/dissectors/packet-xmpptabtab instead of: vi epan/dissectors/packet-xmpptabtab[1]^H^H^H^H^H^H^H^H^H^H^Hxmpptab/tabtab [1] insert grumpy remark here Fair enough. So another tweak to the suggested naming: packet-xmpp/xmpp-whatever.c In fact taking your suggestion of removing packet- from all the file names would also achieve the same thing. I'm not particularly fond of the idea - just being conservative perhaps; but how many subdirectories are acceptable before it gets out of hand - 1000, one for every protocol in WS or a smaller number ;-) I expect most dissectors will stay in the root of epan/dissectors/. My understanding is that this would only be in a few select cases (bluetooth and xmpp are the ones that come to mind) where there is a clear logical grouping of a large number (16 for bluetooth, 14 for xmpp) of files. It makes sense to put them in their own folder. I just gave epan/dissectors/ a quick scan. Here are the other large groupings that immediately stand out: - aim (23 files) - dcerpc (111 files) - gsm (23 files) - h### (33 files) (is there a better generic name for this category?) - ipmi (12 files) - smb (15 files) - zbee (14 files) There are lots and lots of smaller groups of 4-5 files, but I wouldn't expect them to get their own folder. More input is always welcome :) Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Lua linkage with CMake
On Fri, Aug 24, 2012 at 10:43 PM, Evan Huus eapa...@gmail.com wrote:
On Fri, Aug 24, 2012 at 6:52 PM, Tony Trinh ton...@gmail.com wrote:
It's possible that ${wireshark-src}/cmake/modules/FindLUA.cmake is not
properly detecting Lua 5.2 on your system. Check the path defined by
LUA_LIBRARY (and check LUA_INCLUDE_DIR while you're at it), using these
commands:
$ cd ${wireshark-src}
$ cmake -LA | grep LUA
The output should look something like this:
ENABLE_LUA:BOOL=ON
LUA_INCLUDE_DIR:PATH=/Users/tony/src/lua-5.2.1/src/build/include
LUA_LIBRARIES:STRING=/Users/tony/src/lua-5.2.1/src/build/lib/liblua.a
LUA_LIBRARY:FILEPATH=/Users/tony/src/lua-5.2.1/src/build/lib/liblua.a
You can either fix the detection script, or manually edit your path
variables (I use `ccmake`):
Enter the CMake configuration with: $ ccmake .
Press 't' to toggle Advanced mode (to show the advanced variables)
Verify (and edit) LUA_INCLUDE_DIR, LUA_LIBRARIES, and LUA_LIBRARY. For the
library paths, enter the full path to liblua.a for Lua 5.2.
Thanks a bunch, that was exactly what I needed to do - the include
path was pointing to 5.1 for some reason, while the link paths were
correctly pointing to 5.2. Everything works now.
I fixed the auto-detection in revision 44669. It wouldn't find an
include path that had a dot between the 5 and the 2 (ie
/usr/include/lua5.2/), but it would find a library path with a dot,
thus the mismatch in versions.
Evan
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Lua linkage with CMake
Hi all, I recently attempted to switch from autotools to cmake for building wireshark, but for some reason cmake won't link liblua properly: lib/libwireshark.so: undefined reference to `lua_tonumber' lib/libwireshark.so: undefined reference to `lua_call' lib/libwireshark.so: undefined reference to `lua_pcall' lib/libwireshark.so: undefined reference to `luaL_loadfile' lib/libwireshark.so: undefined reference to `luaL_register' If I compile with VERBOSE=1, the failing gcc command is: /usr/bin/gcc -O2 -g -Wall -W -Wextra -Wendif-labels -Wpointer-arith -Warray-bounds -Wcast-align -Wformat-security -fexcess-precision=fast -Wdeclaration-after-statement -Wno-pointer-sign -Wold-style-definition -Wl,--as-needed CMakeFiles/dftest.dir/dftest.c.o CMakeFiles/dftest.dir/ui/util.c.o -o dftest -rdynamic -L/home/eapache/Desktop/wireshark/ui/gtk -L/home/eapache/Desktop/wireshark/ui/qt -L/home/eapache/Desktop/wireshark/codecs -L/home/eapache/Desktop/wireshark/epan -L/home/eapache/Desktop/wireshark/wiretap -L/home/eapache/Desktop/wireshark/wsutil lib/libwireshark.so -lpcap -lcares -lkrb5 -llua5.2 -lGeoIP -lgcrypt -lgpg-error -lgnutls -lsmi -lz -lm lib/libwiretap.so -lgmodule-2.0 -lrt -lz lib/libwsutil.so -lglib-2.0 -Wl,-rpath,/home/eapache/Desktop/wireshark/ui/gtk:/home/eapache/Desktop/wireshark/ui/qt:/home/eapache/Desktop/wireshark/codecs:/home/eapache/Desktop/wireshark/epan:/home/eapache/Desktop/wireshark/wiretap:/home/eapache/Desktop/wireshark/wsutil:/home/eapache/Desktop/wireshark/lib: The switch -llua5.2 does exist in the above line, and is the correct switch, so I guess it's just not late enough in the list of libraries. Knowing nothing about cmake, I thought I'd ask here - where should I be looking to figure out what's gone wrong? Autotools builds (and links with liblua) correctly. Thanks, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Lua linkage with CMake
On Fri, Aug 24, 2012 at 6:52 PM, Tony Trinh ton...@gmail.com wrote:
It's possible that ${wireshark-src}/cmake/modules/FindLUA.cmake is not
properly detecting Lua 5.2 on your system. Check the path defined by
LUA_LIBRARY (and check LUA_INCLUDE_DIR while you're at it), using these
commands:
$ cd ${wireshark-src}
$ cmake -LA | grep LUA
The output should look something like this:
ENABLE_LUA:BOOL=ON
LUA_INCLUDE_DIR:PATH=/Users/tony/src/lua-5.2.1/src/build/include
LUA_LIBRARIES:STRING=/Users/tony/src/lua-5.2.1/src/build/lib/liblua.a
LUA_LIBRARY:FILEPATH=/Users/tony/src/lua-5.2.1/src/build/lib/liblua.a
You can either fix the detection script, or manually edit your path
variables (I use `ccmake`):
Enter the CMake configuration with: $ ccmake .
Press 't' to toggle Advanced mode (to show the advanced variables)
Verify (and edit) LUA_INCLUDE_DIR, LUA_LIBRARIES, and LUA_LIBRARY. For the
library paths, enter the full path to liblua.a for Lua 5.2.
Thanks a bunch, that was exactly what I needed to do - the include
path was pointing to 5.1 for some reason, while the link paths were
correctly pointing to 5.2. Everything works now.
Evan
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
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] Why are authors never Cc'ed before their code is changed?
On Thu, Aug 23, 2012 at 5:23 PM, Lori Jakab lja...@ac.upc.edu wrote: On 08/23/12 05:52, Pascal Quantin wrote: 2012/8/19 Lori Jakab l...@lispmob.org mailto:l...@lispmob.org On 08/19/12 06:56, Pascal Quantin wrote: Having a RSS link (or something similar) to the subversion file log (http://anonsvn.wireshark.org/viewvc/trunk/epan/dissectors/packet-gsmtap.c?view=log for example) could be a way to easily monitor the changes done to a given file for whoever is interested. Of course it would not prevent the offending commit but it would allow a faster feedback / bugfix if needed (which could be sufficient from my point of vue). Would it be an acceptable first step towards improving the monitoring for changes by original authors / contributors? I think that some kind of automatic and *opt-in* method to notify the author(s) of a certain dissector that changes have been made to their code should be available. Both the special header field proposed by Evan (watch) and Sylvain (maintainer) and the RSS feed would fulfill this. And they are not mutually exclusive either. RSS would allow interested non-authors as well to easily follow changes, while the maintainer option could be integrated with bugzilla so that authors are automatically CC'ed on patches submitted for files they maintain. Hi, it appears that Wireshark git mirror (http://http://code.wireshark.org/git/) already has RSS / Atom links (thanks for the hint Jakub ;) ). So whoever is interested can already subscribe to feeds. To get the full commit history you can use: http://code.wireshark.org/git/?p=wireshark;a=atom http://code.wireshark.org/git/?p=wireshark;a=rss And to follow a specific file, you can use the 'f' parameter: http://code.wireshark.org/git/?p=wireshark;a=atom;f=epan/dissectors/packet-gmr1_bcch.c http://code.wireshark.org/git/?p=wireshark;a=rss;f=epan/dissectors/packet-gmr1_bcch.c This is awesome, thanks a lot! I didn't know about the Git repository, are patches in the 'git diff' format against the master branch accepted as well? (The output differs slightly from 'svn diff'.) Not officially, I don't think, but effectively yes. I spent several months submitting git-format patches to bugzilla and never had anyone complain. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Compilation failure - privileges.c: In function 'relinquish_special_privs_perm' - ignoring return value of 'setresgid', declared with attribute warn_unused_result [-Werror=unused-r
On Mon, Aug 20, 2012 at 3:49 PM, Kaul myk...@gmail.com wrote: Recently (~2 days or so ago?), I've failed to compile wireshark, on F17/x64: libtool: compile: gcc -DHAVE_CONFIG_H -I. -I.. -I./.. -DINET6 -DG_DISABLE_DEPRECATED -DG_DISABLE_SINGLE_INCLUDES -DGSEAL_ENABLE -DGTK_DISABLE_DEPRECATED -DGTK_DISABLE_SINGLE_INCLUDES -D_U_=__attribute__((unused)) -D_FORTIFY_SOURCE=2 -I/usr/local/include -DPLUGIN_DIR=\/usr/local/lib/wireshark/plugins/1.9.0\ -Werror -g -O2 -Wall -W -Wextra -Wdeclaration-after-statement -Wendif-labels -Wpointer-arith -Wno-pointer-sign -Warray-bounds -Wcast-align -Wformat-security -Wold-style-definition -Wno-error=unused-but-set-variable -fexcess-precision=fast -pthread -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng15 -MT privileges.lo -MD -MP -MF .deps/privileges.Tpo -c privileges.c -fPIC -DPIC -o .libs/privileges.o privileges.c: In function 'relinquish_special_privs_perm': privileges.c:275:12: error: ignoring return value of 'setresgid', declared with attribute warn_unused_result [-Werror=unused-result] privileges.c:282:12: error: ignoring return value of 'setresuid', declared with attribute warn_unused_result [-Werror=unused-result] cc1: all warnings being treated as errors If it were git, I'm sure I could easily use 'git bisect' and find the issue. Without it, I'm really unsure - as the last change to this file took place some time ago. We do have a read-only git mirror at [1]. If the last change to the file happened a while ago (presumably long before the issue started appearing) then it's likely your toolchain changed recently. Did you install a gcc or libc update in the last couple of days? Not that it isn't our fault (assuming the warning isn't a false positive), it's just likely that only the most recent gcc/libc toolchain will warn on the problem. Thanks for the report, Evan [1] http://code.wireshark.org/git/wireshark ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] ep_ memory is not garbage collected
On Fri, Aug 17, 2012 at 10:02 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Thu, Aug 16, 2012 at 08:54:54PM -0400, Evan Huus wrote: On Wed, Aug 15, 2012 at 11:36 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: From r43188 sequence of selecting new packet (cf_select_packet() in file.c) is: cf_read_frame() old_edt = cf-edt; cf-edt = epan_dissect_new(); edt_refs++ epan_dissect_run(cf-edt, ...) if (old_edt) epan_dissect_free(old_edt); ; edt_refs-- (edt_refs != 0, no gc) Old code was doing something like: cf_read_frame() epan_dissect_free(cf-edt); ; edt_refs--(likely edt_refs == 0, gc ep memory) cf-edt = epan_dissect_new() ; edt_refs++ I have working fix for this, but check below. Based on the log for r43188 I expect there's someway to force clear the GtkTreeStore, so that old_edt can be unrefed (triggering a gc) before cf-edt is re-allocated? Is your fix checked in or attached to a bug somewhere? Attached, but it's more workaround than fix. Looks sane enough as far as workarounds go. I don't know if there's already a process for this, but I would commit that to trunk (with a comment in the code pointing to this discussion). Once it's had a week or two of soak time, backport it to 1.8 and remove it from trunk. Any objections from the general list? I'll try and take a look at a proper fix for trunk at some point, but I've been very busy with real life lately, so no promises. I'm pretty sure the stack idea from my previous email works if somebody else wants to grab this. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Why are authors never Cc'ed before their code is changed?
On Sun, Aug 19, 2012 at 7:03 AM, Harald Welte lafo...@gnumonks.org wrote: Hi all, I understand that different FOSS projects have different cultures, norms, rules, etc. However, my experience with wireshark it has reached a point where I think a post like this is requierd. I don't want to see this as some kind of flame, or to claim that the wireshark development model is bad. I just really don't understand it, coming from a different background. Please help me understand it. In other large projects (let's say the Linux kernel), it is customary that the original author of code (or a designated maintainer who has taken over that particular module) is always Cc'ed before a change to his code is being made. In wireshark, it has happened repeatedly, that code contributed by Osmocom developers (like the GMR dissector of Sylvain Munaut, or my GSMTAP dissector) has been modified erroneously (and thus broken) without any notice to us. I find this at least disturbing (if not annoying) adn am wondering what is the benefit of this practise to wireshark or its users? It is generally a fair assumption to make that somebody who writes a particular dissector actually cares about that code being functional, and that he probably knows the respective protocol quite well. In most caess, I would expect that author to be able to review changes to his code. So why are those authors not Cc'ed in any kind of patches, or merge requests to their code? If you don't want to wait for their explicit approval for efficiency reasons, you could at least notify them that there was a change to their code that they should review. The current situation ends up like this: * People like me who just contribute particular dissectors have no time to go through all of the committlog or read all of the mailing list. * Somebody else quietly makes a change, without discussing the change beforehand, without Cc'ing the proposed change to us * A wireshark developer committs that patch, again without Cc'ing the original author * Wireshark ends up being broken for the given protocol * This is not discovered for a long time, until one of the few 'bleedign edge' users of those protocols discovers a problem and finds the time to report it, at which point we get the complaint about something not working and have to go back in history. I would like to raise the following questions: 1) Is this the way how the wireshark development model / flow is supposed to work ? 2) If yes, do you really think that the gain in flexibilty caused by this anarchy overweighs the benefit of having dissector-authors give timely feedback to proposed changes, or prevent breakage? 3) Do you have any idea how to improve this situation? 4) How do other wireshark dissector authors deal with this problem? Thanks in advance! Regards, Harald Hi Harald, I've not been involved with Wireshark development for very long now, but there are a few things that I expect are contributing to this problem. Mainly, there's no automated method for sending out those emails. Wireshark code tends to see a lot of churn both in minor improvements, bugfixes, and architectural changes. Requiring developers to send out emails for each change would add a lot of overhead to the workflow. We have enough trouble keeping up with bug reports as it is. Also, while your assumption that the original author cares is probably true in a lot of cases, it's not true in all of them, and getting regular emails for the rest of your dissector's life could be very spammy/annoying to some people. I do agree that it would be a good thing if original authors could be notified if they wanted to be. Perhaps it would be possible to add a special 'watch' line (like the $Id$ line) to the comment header, and have an SVN post-commit hook that sends an email to all the addresses listed in that watch line for each changed file? Then dissector authors can watch their own files (and any others they care about) and they'll automatically receive a notification when those files change. Hope this provides some explanation, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] [Wireshark-commits] buildbot failure in Wireshark (development) on Windows-XP-x86
The XP-x86 buildbot has been failing sporadically for over a week now. It seems to have started around revision 44359, but the actual cause could be any number of revisions on either side (I don't have an XP box to bisect with). Anyone have an idea what's happening? On Sun, Aug 19, 2012 at 11:22 AM, buildbot-no-re...@wireshark.org wrote: The Buildbot has detected a new failure on builder Windows-XP-x86 while building Wireshark (development). Full details are available at: http://buildbot.wireshark.org/trunk/builders/Windows-XP-x86/builds/2229 Buildbot URL: http://buildbot.wireshark.org/trunk/ Buildslave for this Build: windows-xp-x86 Build Reason: scheduler Build Source Stamp: 44582 Blamelist: eapache,gerald BUILD FAILED: failed test.sh sincerely, -The Buildbot ___ Sent via:Wireshark-commits mailing list wireshark-comm...@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-commits Unsubscribe: https://wireshark.org/mailman/options/wireshark-commits mailto:wireshark-commits-requ...@wireshark.org?subject=unsubscribe ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] ep_ memory is not garbage collected
On Wed, Aug 15, 2012 at 11:36 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: On Wed, Aug 15, 2012 at 10:22:13AM -0400, Evan Huus wrote: On Wed, Aug 15, 2012 at 9:53 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: From commit r42254 (reference counting of edt) we have problem with ep_ memory not being returned to pool after dissecting packet. Wireshark after initial opening file, always keep edt for currently selected packet, so edt_refs is always 0. You can reproduce it by loading some bigger files, and several times open expert or conversation window. After r43188 (not part of wireshark-1.8) affected is also filtering (if at least one packet match filter), or selecting other packets. I'm not sure I follow. Are we calling epan_dissect_run() for each packet selected without calling epan_dissect_cleanup()/epan_dissect_init() in between? From r43188 sequence of selecting new packet (cf_select_packet() in file.c) is: cf_read_frame() old_edt = cf-edt; cf-edt = epan_dissect_new(); edt_refs++ epan_dissect_run(cf-edt, ...) if (old_edt) epan_dissect_free(old_edt); ; edt_refs-- (edt_refs != 0, no gc) Old code was doing something like: cf_read_frame() epan_dissect_free(cf-edt); ; edt_refs--(likely edt_refs == 0, gc ep memory) cf-edt = epan_dissect_new() ; edt_refs++ I have working fix for this, but check below. Based on the log for r43188 I expect there's someway to force clear the GtkTreeStore, so that old_edt can be unrefed (triggering a gc) before cf-edt is re-allocated? Is your fix checked in or attached to a bug somewhere? I understand that to be a bug in the caller, not a bug in ep_'s memory manager, or did r42254 change the behaviour of the API in an unexpected way? cf_select_packet() don't call epan_dissect_free() but holds edt reference in cf-edt, so if we have selected packet than edt_refs 0 (this is also true before r43188). Yes, I understand now. If in such situation we do something which requires redissecting lot of packet (or do it several times) wireshark will eat all your memory ;-) I think it'd be best if each dissection have unique, single ep_pool (no ref-counting), but it's lot of work, so we need some workaround, also for 1.8 ;/ Agreed that this is desirable. Not 100% sure how much work it will be - does it make sense to attach the ep_ memory pool to the edt struct, and then just have a global stack whose top tracks the 'active' edt? I was going to suggest just a global var to track the active edt, but remembered that some weird nesting conditions were what required the ref-counting in the first place. I think a stack should take care of that? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Passing NULL to %s format specifiers
On Thu, Aug 16, 2012 at 1:16 PM, Gerald Combs ger...@wireshark.org wrote: On 8/15/12 8:12 AM, Evan Huus wrote: On Wed, Aug 15, 2012 at 10:15 AM, Jeff Morriss jeff.morriss...@gmail.com wrote: Evan Huus wrote: On Linux and most other operating systems I know of, passing a NULL to a %s format specifier is safe. On Solaris, as it turns out, it isn't [1]. It's a little more complex than that. The problem is present if the system's C library doesn't handle passing NULL to %s AND GLib wasn't compiled with --enable-included-printf. This used to be the case for Windows but was fixed a couple of years ago. It's still broken for Solaris because the default for enable-included-printf is auto, at least according to the GLib sources. It seems like it should be yes in order to provide consistent behavior across platforms or at least check the behavior of passing NULL to %s. Note that you can still segfault with printf(%s, NULL) on Linux since gcc will use its builtin printf in that case. Very interesting, thanks for the details. I've added a short note to README.developer, so hopefully new code at least will be safe. I'm a fan of a macro like Jakub mentioned as part of the old conversation: http://www.wireshark.org/lists/wireshark-dev/201105/msg00205.html If we go that route, perhaps someone can add a bit to checkAPIs that complains if it finds %s in a format string without the macro? I'm OK with this but it seems like we should be able to get Coverity or Clang to do the work for us. They're both pretty good at finding NULL pointer dereferences. I know that CppCheck supports writing custom rules as XML files and regexs (although I know nothing more about it at the moment then that). It may be possible to subsume all of the current checkAPIs into CppCheck rules, which would probably be a good thing since then we'd get a proper grammar engine for free instead of relying on perl hacks. Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] ep_ memory is not garbage collected
On Wed, Aug 15, 2012 at 9:53 AM, Jakub Zawadzki darkjames...@darkjames.pl wrote: From commit r42254 (reference counting of edt) we have problem with ep_ memory not being returned to pool after dissecting packet. Wireshark after initial opening file, always keep edt for currently selected packet, so edt_refs is always 0. You can reproduce it by loading some bigger files, and several times open expert or conversation window. After r43188 (not part of wireshark-1.8) affected is also filtering (if at least one packet match filter), or selecting other packets. I'm not sure I follow. Are we calling epan_dissect_run() for each packet selected without calling epan_dissect_cleanup()/epan_dissect_init() in between? I understand that to be a bug in the caller, not a bug in ep_'s memory manager, or did r42254 change the behaviour of the API in an unexpected way? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Passing NULL to %s format specifiers
On Wed, Aug 15, 2012 at 10:15 AM, Jeff Morriss jeff.morriss...@gmail.com wrote: Evan Huus wrote: On Linux and most other operating systems I know of, passing a NULL to a %s format specifier is safe. On Solaris, as it turns out, it isn't [1]. The case in the filed bug is fairly trivial to fix, but I'm wondering if this is something that should be added to the Code Style / Portability section of README.developer? Alternatively, since I have no idea how many of these bugs we may have to fix, perhaps we should be wrapping all format strings somehow? Hopefully someone with some Solaris experience (or even a Solaris test box) could weigh in, since I have neither. For the record we last discussed this a little over a year ago: http://www.wireshark.org/lists/wireshark-dev/201105/msg00202.html As for adding something to the doc: sure but I doubt it'll help much. I'm a fan of a macro like Jakub mentioned as part of the old conversation: http://www.wireshark.org/lists/wireshark-dev/201105/msg00205.html If we go that route, perhaps someone can add a bit to checkAPIs that complains if it finds %s in a format string without the macro? Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Passing NULL to %s format specifiers
On Linux and most other operating systems I know of, passing a NULL to a %s format specifier is safe. On Solaris, as it turns out, it isn't [1]. The case in the filed bug is fairly trivial to fix, but I'm wondering if this is something that should be added to the Code Style / Portability section of README.developer? Alternatively, since I have no idea how many of these bugs we may have to fix, perhaps we should be wrapping all format strings somehow? Hopefully someone with some Solaris experience (or even a Solaris test box) could weigh in, since I have neither. Cheers, Evan [1] https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=7634 ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org 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] Warnings when loading preferences
On Wed, Aug 1, 2012 at 9:55 AM, Kurt Knochner ws.dev.l...@nospam.knochner.com wrote: would it make sense to add version sections to the preferences file? Parsing the preference file would a bit more work, but one could separate the modified preferences in a proper way from each other. Extra benefit: One could use the same preference file with different (older) versions of Wireshark, without any problem (if such a feature get's backported). Something like this: version: 1.6.0 ... /version: 1.6.0 version: 1.8.0 ... /version: 1.8.0 Regards Kurt Very neat idea. It would probably make more sense to split into minversion (for new preferences introduced in a certain release) and maxversion (for preferences deprecated in a certain release) but the concept is cool. It wouldn't solve this particular problem unfortunately, because 1.8 and prior don't have support for those tags and would throw a warning (or an error?) if it saw them. It would be nice to have some version of this going forward for 1.10+ though. Cheers, Evan ___ Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
