P.S. And one last item for the list of tangents fitting the theme, I knew I'd missed something:
- p_add_proto_data shouldn't be stored in frame_data, since those have file scope not session scope; following this logic it should be stored in epan_session_t somehow (hash table keyed by frame id and proto id? the sorted GSList method has always confused me, I'm too tired to make algorithmic sense of a sorted singly-linked list...) On Thu, Nov 21, 2013 at 9:59 PM, Evan Huus <[email protected]> wrote: > On Thu, Nov 21, 2013 at 7:02 PM, <[email protected]> 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 <[email protected]> >> To: Developer support list for Wireshark <[email protected]> >> 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 <[email protected]> wrote: >>> >>> On Nov 21, 2013, at 12:08 PM, [email protected] 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 <[email protected]> >> Archives: http://www.wireshark.org/lists/wireshark-dev >> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev >> mailto:[email protected]?subject=unsubscribe >> >> >> ___________________________________________________________________________ >> Sent via: Wireshark-dev mailing list <[email protected]> >> Archives: http://www.wireshark.org/lists/wireshark-dev >> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev >> mailto:[email protected]?subject=unsubscribe ___________________________________________________________________________ Sent via: Wireshark-dev mailing list <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
