Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 53489: /trunk/epan/ /trunk/epan/diss
On Thu, Nov 21, 2013 at 7:02 PM, <mmann78@xxxxxxxxxxxx> wrote:
> Should there be separate APIs or should p_add_proto_data have a memory
> allocator function and packet_scope() would go to info->pool and everything
> else (NULL or file_scope) would go to where it is now?
The frame_data API currently uses a GSList in the frame_data struct,
so I think you'd need to add a GSList to packet_info and duplicate the
entire API :(
---
Architectural ramblings follow, prompted by this discussion.
---
Thinking a little more broadly about our current data-storage, we have
a number of different lifetimes and dimensions.
We might want to store data with any of the following lifetimes:
- libwireshark: the lifetime of the program, aka epan_scope
- open file: the lifetime of the open file (even across redissections
and pref changes); the only thing we have for this right now is the
cfile struct and frame_data structs I think, though they're basically
global (thus why we can only open one file per process)
- dissection session: epan_session_t struct and file_scope (which is a
misleading name, in hindsight); effectively the file, except it resets
when prefs change and we have to redissect everything
- active packet: epan_dissect_t struct, packet_info struct and
pinfo->pool, basically the lifetime of the packet selected in the UI
- dissected packet: packet_scope, ie just until the principle
dissection is finished, so this gets cleaned up even when the packet
is still selected in the UI
We might also want to store data according to any of the following dimensions:
- per file (if we ever support more than one open at a time)
- per protocol
- per conversation
- per packet
We currently have storage options for:
- session-lifetime per-conversation per-protocol (conversation_add_proto_data)
- session-lifetime per-packet per-protocol (p_add_proto_data, though
note that this is stored in the frame_data struct, which itself has a
file lifetime, not a session lifetime, we workaround this by calling
frame_data_reset on *all* our frame_data's when session scope changes)
- active-packet-lifetime per-packet (though it's not a generic API,
this is basically what all the random pinfo fields were for - rather
than casting to a void* people just added a field as needed)
And you are looking for a way to do active-packet-lifetime per-packet
per-protocol storage in a generic way, which we don't really have
right now since pinfo's a mess.
Presumably a nice clean architecture would have exactly one structure
type and one memory pool for each of the possible lifetimes, and a
lifetime-agnostic way to store data according to any of the various
dimensions.
This is one of the pipe-dreamy ideas I had in mind when designing wmem
(since emem didn't give us epan_scope() or pinfo->pool), and I think
is possibly one of the aspects Jakub was working on when he did the
epan_session_t and epan_dissect_t structs.
A collection of little things that may or may not be good ideas but
fit the theme:
- One implication of the above is that eventually the cfile struct
might get its own wmem pool, as it's the only lifetime without a wmem
pool now. I imagine the work involved to make cfile non-global is
still immense (though I think Gerald's put some work into fixing this
for qtshark) but having a common memory pool for it that's distinct
from epan_scope() may help?
- The file_scope() pool should maybe be renamed to session_scope()
since it doesn't necessarily last the entire lifetime of the file?
Eventually this pool should be a member of the epan_session_t struct
and should be passed around enough that it won't have to be a global.
- Once pinfo has been shrunk sufficiently, it should probably be
merged with epan_dissect_t struct since they appear to have the same
lifetime right now.
---
I have no idea how coherent the above turned out, but hopefully it
provides some food for thought. Corrections and criticisms more than
welcome, I'm sufficiently tired that I'm sure there must be at least
one inaccuracy somewhere. Now back to schoolwork...
Cheers,
Evan
> -----Original Message-----
> From: Evan Huus <eapache@xxxxxxxxx>
> To: Developer support list for Wireshark <wireshark-dev@xxxxxxxxxxxxx>
> Sent: Thu, Nov 21, 2013 6:30 pm
> Subject: Re: [Wireshark-dev] [Wireshark-commits] rev 53489: /trunk/epan/
> /trunk/epan/dissectors/: packet-ethertype.c packet-exported_pdu.c
> packet-infiniband.c packet-mpls.c packet-pw-eth.c packet-sctp.c
> /trunk/epan/: exported_pdu.c exported_pdu.h ...
>
> On Thu, Nov 21, 2013 at 3:44 PM, Guy Harris <guy@xxxxxxxxxxxx> wrote:
>>
>> On Nov 21, 2013, at 12:08 PM, mmann@xxxxxxxxxxxxx wrote:
>>
>>> http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=53489
>>>
>>> User: mmann
>>> Date: 2013/11/21 08:08 PM
>>>
>>> Log:
>>> Remove ethertype, mpls_label and ppids from packet_info structure.
>>
>>>
>>> The information was converted to "proto" data within their respective
> dissectors strictly for use in "Decode As".
>>
>> "proto" data is persistent, so you're allocating a chunk of data for every
> packet in an Ethernet capture, for example, which remains around until the
> capture is closed. That might amount to a significant additional amount of
> memory for a large capture.
>>
>> Perhaps what's needed here is a way for dissectors to attach arbitrary
>> data to
> a packet_info structure, with the data being freed when the packet_info
> structure is freed (for example, when the epan_dissect_t containing a
> packet_info structure is freed).
>
> Memory allocated in the pinfo wmem allocator (pinfo->pool) will
> automatically be freed when the packet_info struct is freed. It would
> be pretty easy to do an analogue of p_add_proto_data using pinfo and
> this wmem pool.
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
>
>
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
