Wireshark-dev: Re: [Wireshark-dev] Replacing wmem_packet_scope() with pinfo->pool?

From: João Valverde <joao.valverde@xxxxxxxxxxxxxxxxxx>
Date: Mon, 12 Jul 2021 20:06:54 +0100


On 12/07/21 19:48, Evan Huus wrote:
On Mon, Jul 12, 2021 at 14:42 João Valverde via Wireshark-dev <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>> wrote:



    On 12/07/21 19:13, Evan Huus wrote:
     > On Mon, Jul 12, 2021 at 2:05 PM João Valverde via Wireshark-dev
     > <wireshark-dev@xxxxxxxxxxxxx
    <mailto:wireshark-dev@xxxxxxxxxxxxx>> wrote:
     >>
     >>
     >>
     >> On 12/07/21 16:52, Evan Huus wrote:
     >>> I've been thinking recently about starting the process of
    getting rid
     >>> of the "global" wmem scope methods (wmem_packet_scope,
     >>> wmem_file_scope, etc) in favour of passing them around in arguments
     >>> (or in pinfo, or something). This would let us drop a bunch of
     >>> in-scope/out-of-scope tracking and assertion, as well as make
    the code
     >>> more amenable to future refactors like (potentially) concurrency.
     >>>
     >>> At a first glance, we already have pinfo->pool which maintains the
     >>> lifetime of the packet_info object. As far as I can reason, this is
     >>> almost/effectively the same as the existing wmem_packet_scope - it
     >>> gets cleaned up later in the dissection flow, but there's still
    only
     >>> ever one which gets reused for each packet.
     >>>
     >>> Is this correct? If so, does it make sense to start replacing
     >>> `wmem_packet_scope()` calls with `pinfo->pool` when pinfo is
    already
     >>> in scope?
     >>
     >> I think wmem_packet_scope() should return pinfo->pool.
     >
     > It would have to be converted to a macro (or do a mass-replace anyway
     > to take pinfo as an argument), so I figure using `pinfo->pool`
     > directly in most cases ends up being simplest.

    I really don't see it being simplest.


Why?

The motivation for this change is primarily to get rid of the global methods so that scope management becomes easier, and so that we could e.g. have two packet scopes active at once in a future where we do parallel dissection.

OK, I withdraw my objection then. I agree it gets us closer to that goal.

Having wmem_packet_scope return pinfo->pool doesn’t really accomplish either of those goals.

    Please reconsider.

    Either wmem_packet_scope() is created earlier and pinfo->pool =
    wmem_packet_scope() or wmem_enter_packet_scope() is passed pinfo->pool
    and packet_scope = pinfo->pool.

    Either way works fine AFAICT.

     >> Other than that, I don't see a compelling reason to remove the
    global
     >> wmem scope methods.
     >>
     >>> Thanks,
     >>> Evan
     >>>
    ___________________________________________________________________________
     >>> Sent via:    Wireshark-dev mailing list
    <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>>
     >>> Archives: https://www.wireshark.org/lists/wireshark-dev
    <https://www.wireshark.org/lists/wireshark-dev>
     >>> Unsubscribe:
    https://www.wireshark.org/mailman/options/wireshark-dev
    <https://www.wireshark.org/mailman/options/wireshark-dev>
     >>>                mailto:wireshark-dev-request@xxxxxxxxxxxxx
    <mailto:wireshark-dev-request@xxxxxxxxxxxxx>?subject=unsubscribe
     >>>
     >>
    ___________________________________________________________________________
     >> Sent via:    Wireshark-dev mailing list
    <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>>
     >> Archives: https://www.wireshark.org/lists/wireshark-dev
    <https://www.wireshark.org/lists/wireshark-dev>
     >> Unsubscribe:
    https://www.wireshark.org/mailman/options/wireshark-dev
    <https://www.wireshark.org/mailman/options/wireshark-dev>
     >>               mailto:wireshark-dev-request@xxxxxxxxxxxxx
    <mailto:wireshark-dev-request@xxxxxxxxxxxxx>?subject=unsubscribe
     >
    ___________________________________________________________________________
     > Sent via:    Wireshark-dev mailing list
    <wireshark-dev@xxxxxxxxxxxxx <mailto:wireshark-dev@xxxxxxxxxxxxx>>
     > Archives: https://www.wireshark.org/lists/wireshark-dev
    <https://www.wireshark.org/lists/wireshark-dev>
     > Unsubscribe:
    https://www.wireshark.org/mailman/options/wireshark-dev
    <https://www.wireshark.org/mailman/options/wireshark-dev>
     >               mailto:wireshark-dev-request@xxxxxxxxxxxxx
    <mailto:wireshark-dev-request@xxxxxxxxxxxxx>?subject=unsubscribe
     >

    ___________________________________________________________________________
    Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx
    <mailto:wireshark-dev@xxxxxxxxxxxxx>>
    Archives: https://www.wireshark.org/lists/wireshark-dev
    <https://www.wireshark.org/lists/wireshark-dev>
    Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
    <https://www.wireshark.org/mailman/options/wireshark-dev>
                  mailto:wireshark-dev-request@xxxxxxxxxxxxx
    <mailto:wireshark-dev-request@xxxxxxxxxxxxx>?subject=unsubscribe


___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
              mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe