Re: [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 ...

[Evan Huus <eapache () gmail com>]

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 () gmail com> wrote:
> On Thu, Nov 21, 2013 at 7:02 PM,  <mmann78 () netscape net> 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 () gmail com>
> > To: Developer support list for Wireshark <wireshark-dev () wireshark org>
> > 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 () alum mit edu> wrote:
> > > On Nov 21, 2013, at 12:08 PM, mmann () wireshark org 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 () wireshark org>
> > Archives:    http://www.wireshark.org/lists/wireshark-dev
> > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> >              mailto:wireshark-dev-request () 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-request () 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-request () wireshark org?subject=unsubscribe