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 <eapache@xxxxxxxxx> wrote:
> 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
