Re: [Wireshark-dev] [Wireshark-commits] rev 52264: /trunk/ /trunk/epan/: value_string.c value_string.h /trunk/: rawshark.c
On Sun, Sep 29, 2013 at 8:44 AM, wrote: > http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=52264 > > User: eapache > Date: 2013/09/29 12:44 PM > > Log: > Replace some val_to_str calls with the equivalent val_to_str_const calls (and > implement rval_to_str_const to do this). The format-strings didn't have any > parameter specifiers in them, so they were clearly never used (or they would > have blown up) but still a bug. > > This is one of the first steps towards converting val_to_str and friends to > wmem. I'm honestly not sure what the best approach is for the API in this > case: > the vast majority of usage is within dissectors, so just hard-coding packet > scope (the way they currently hard-code ep_ scope) doesn't look terrible, but > there are *some* uses in taps and other places that will need to be > converted to > something else if we go that route. Adding a wmem_pool parameter just for the > uncommon case seems a bit like overkill, though perhaps it is the right > thing to > do. Thoughts on this? ___ Sent via:Wireshark-dev mailing list Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] [Wireshark-commits] rev 52264: /trunk/ /trunk/epan/: value_string.c value_string.h /trunk/: rawshark.c
On Sun, Sep 29, 2013 at 08:46:23AM -0400, Evan Huus wrote: > On Sun, Sep 29, 2013 at 8:44 AM, wrote: > > http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=52264 > > > > User: eapache > > Date: 2013/09/29 12:44 PM > > > > Log: [...] > > This is one of the first steps towards converting val_to_str and friends to > > wmem. I'm honestly not sure what the best approach is for the API in this > > case: > > the vast majority of usage is within dissectors, so just hard-coding packet > > scope (the way they currently hard-code ep_ scope) doesn't look terrible, > > but > > there are *some* uses in taps and other places that will need to be > > converted to > > something else if we go that route. Adding a wmem_pool parameter just for > > the > > uncommon case seems a bit like overkill, though perhaps it is the right > > thing to > > do. > > Thoughts on this? Most of current calls are done by col_append_fstr() or expert_add_info_format() so it'd be actually great to have own formatting conversion. /me smiles to register_printf_function(), still not portable ;| So... What about removing val_to_str(), val_to_str_const() rename try_val_to_str() to val_to_str() make caller responsible to do what he want when there's no value for value_string entry. value_string API don't allocates memory -> no problem ;] But back to topic (cause you'll probably see this problem few more times). I don't quite get a point why we need to change everything to wmem. (To be honest I still don't quite get why we need wmem_ at all, but let's skip that). But if we really want to do that (change everything to wmem_), we NEED some ep-like temporary pool (which will work both for UI and dissection), or some function which will return packet-pool or gui-pool if there's no dissection. Otherwise we need to remove some functionality. Still I don't see any UI-wmem-pool, do we have one? PS: I know it's obvious, sorry ;| ___ Sent via:Wireshark-dev mailing list Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
[Wireshark-dev] Motivation for wmem [was: rev 52264]
On Sun, Sep 29, 2013 at 3:56 PM, Jakub Zawadzki wrote: > But back to topic (cause you'll probably see this problem few more times). > I don't quite get a point why we need to change everything to wmem. > (To be honest I still don't quite get why we need wmem_ at all, but let's > skip that). Wmem is mostly a replacement for emem, because emem was becoming impossible to maintain and reason about (recall, for example, bug #5284). If you could still deal with emem, congratulations, but the rest of us mortals needed something more comprehensible. (In architectural terms, wmem makes the memory scope, allocator logic, and data structures all orthogonal to each other.) > But if we really want to do that (change everything to wmem_), we NEED some > ep-like temporary pool (which will work both for UI and dissection), > or some function which will return packet-pool or gui-pool if there's no > dissection. Otherwise we need to remove some functionality. No. This is one of the things I dislike most about emem. Using ep_ memory outside of packet dissection provides no guarantees at all as to when that memory is going to be freed, and makes it difficult if not impossible to reason about scope. The majority of such uses I expect to be converted to manually-managed memory. If there is some obvious common UI-scope then we can easily create a new pool for that memory, but the cases I have reviewed have not had any obvious common scope, they are simply using ep_ and assuming that it goes away at some point (and not while they still need it!) > Still I don't see any UI-wmem-pool, do we have one? No, but it's trivial to create one *if* we can determine a sane scope for it. I would love to be able to do this, I just haven't been able to figure out how. Cheers, Evan P.S. I have a pleasant day-dream where libwireshark gets rewritten in a garbage-collected language like Go, but I somehow suspect that isn't going to happen... ___ Sent via:Wireshark-dev mailing list Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Motivation for wmem [was: rev 52264]
On Sun, Sep 29, 2013 at 05:35:59PM -0400, Evan Huus wrote: > On Sun, Sep 29, 2013 at 3:56 PM, Jakub Zawadzki > wrote: > > But back to topic (cause you'll probably see this problem few more times). > > I don't quite get a point why we need to change everything to wmem. > > (To be honest I still don't quite get why we need wmem_ at all, but let's > > skip that). > > Wmem is mostly a replacement for emem, because emem was becoming > impossible to maintain and reason about (recall, for example, bug > #5284). If you could still deal with emem, congratulations, but the > rest of us mortals needed something more comprehensible. Evan, I *really* don't see what wmem_ is fixing. I mostly see that we have few extra assertions to dissallow use it when not dissecting packets. But if we want to remove totally ep_ we'd need to fix this shit anyway. I'm not fan of this, sorry. Right now wmem_ is quite long in our eco system, so this talk is pointless. Please, just let's skip this discussion. I'll make some comments from time to time (/me just being a little conservative), but I think you can life with it? :-) Eventually I might try to lobby replacing wmem_packet_scope() with some pinfo_current()->pool [or pinfo->pool if we have pinfo pointer] - that's all. > > But if we really want to do that (change everything to wmem_), we NEED some > > ep-like temporary pool (which will work both for UI and dissection), > > or some function which will return packet-pool or gui-pool if there's no > > dissection. Otherwise we need to remove some functionality. > > No. This is one of the things I dislike most about emem. Using ep_ > memory outside of packet dissection provides no guarantees at all as > to when that memory is going to be freed, and makes it difficult if > not impossible to reason about scope. The majority of such uses I > expect to be converted to manually-managed memory. If there is some > obvious common UI-scope then we can easily create a new pool for that > memory, but the cases I have reviewed have not had any obvious common > scope, they are simply using ep_ and assuming that it goes away at > some point (and not while they still need it!) Ok, this can be fixed with g_free() after val_to_str_[dup|format]. I'm fine with this. +1 from me. > P.S. I have a pleasant day-dream where libwireshark gets rewritten in > a garbage-collected language like Go, but I somehow suspect that isn't > going to happen... Why Go? if we talk about better languages I'd rather use D. If we talk about glib-language-environment than Vala has reference couting. Still there's popular GC for C: http://www.hpl.hp.com/personal/Hans_Boehm/gc/ and there's alredy interested in using that: http://www.wireshark.org/lists/wireshark-dev/201210/msg00229.html ___ Sent via:Wireshark-dev mailing list Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
Re: [Wireshark-dev] Motivation for wmem [was: rev 52264]
On Sun, Sep 29, 2013 at 6:38 PM, Jakub Zawadzki wrote: > On Sun, Sep 29, 2013 at 05:35:59PM -0400, Evan Huus wrote: >> On Sun, Sep 29, 2013 at 3:56 PM, Jakub Zawadzki >> wrote: >> > But back to topic (cause you'll probably see this problem few more times). >> > I don't quite get a point why we need to change everything to wmem. >> > (To be honest I still don't quite get why we need wmem_ at all, but let's >> > skip that). >> >> Wmem is mostly a replacement for emem, because emem was becoming >> impossible to maintain and reason about (recall, for example, bug >> #5284). If you could still deal with emem, congratulations, but the >> rest of us mortals needed something more comprehensible. > > Evan, I *really* don't see what wmem_ is fixing. Then I must be doing an absolutely awful job of explaining. I apologize, I'm just not sure how else to approach it. > I mostly see that we have few > extra assertions to dissallow use it when not dissecting packets. > But if we want to remove totally ep_ we'd need to fix this shit anyway. > > I'm not fan of this, sorry. Right now wmem_ is quite long in our eco system, > so this talk is pointless. Please, just let's skip this discussion. > I'll make some comments from time to time (/me just being a little > conservative), > but I think you can life with it? :-) I wish we were all on the same page, I think that if we could come to a common understanding then things would go more smoothly. > Eventually I might try to lobby replacing wmem_packet_scope() with some > pinfo_current()->pool [or pinfo->pool if we have pinfo pointer] This is part of the plan I have for wmem, definitely. It's just not as high priority for now. > - that's all. > >> > But if we really want to do that (change everything to wmem_), we NEED >> > some ep-like temporary pool (which will work both for UI and dissection), >> > or some function which will return packet-pool or gui-pool if there's no >> > dissection. Otherwise we need to remove some functionality. >> >> No. This is one of the things I dislike most about emem. Using ep_ >> memory outside of packet dissection provides no guarantees at all as >> to when that memory is going to be freed, and makes it difficult if >> not impossible to reason about scope. The majority of such uses I >> expect to be converted to manually-managed memory. If there is some >> obvious common UI-scope then we can easily create a new pool for that >> memory, but the cases I have reviewed have not had any obvious common >> scope, they are simply using ep_ and assuming that it goes away at >> some point (and not while they still need it!) > > Ok, this can be fixed with g_free() after val_to_str_[dup|format]. > I'm fine with this. +1 from me. > >> P.S. I have a pleasant day-dream where libwireshark gets rewritten in >> a garbage-collected language like Go, but I somehow suspect that isn't >> going to happen... > > Why Go? if we talk about better languages I'd rather use D. > If we talk about glib-language-environment than Vala has reference couting. Because I already know Go :) It was just an idle thought, not a serious proposal. > Still there's popular GC for C: http://www.hpl.hp.com/personal/Hans_Boehm/gc/ > and there's alredy interested in using that: > http://www.wireshark.org/lists/wireshark-dev/201210/msg00229.html I have not looked closely, but I think it would be easy to cause memory leaks by embedding memory addresses in packet bytes. Maybe worth experimenting with though. ___ Sent via:Wireshark-dev mailing list Archives:http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
