Re: [dpdk-dev] [PATCH v2 0/4] Introduce IF proxy library

[Andrzej Ostruszka <amo@semihalf.com>]

On 3/26/20 1:41 PM, Andrzej Ostruszka wrote:
> Thank you David for taking time to look at this.
> 
> On 3/25/20 9:08 AM, David Marchand wrote:
>> Hello Andrzej,
>>
>> On Tue, Mar 10, 2020 at 12:11 PM Andrzej Ostruszka
> [...]
>> I can see we end up exposing structures for registering callbacks.
> 
> Right.  I was thinking more in terms of user convenience so it seemed
> like a good choice to gather them in one struct and call 'register'
> once.  The fact that the same structure is used to keep them is an
> implementation choice and this can be decoupled.
> 
>> Did you consider some ways to avoid exposure of those? (thinking of
>> ABI maintenance for when this library will elect to non-experimental).
> 
> I will.  So far I used the union for the input since I like when things
> are well typed :) and there is no need for casting.  However I will
> spend some time on this and will get back to you soon (if you have
> already something in your head please share).  Right now I'm thinking
> about taking array of callbacks with each entry being ("event type",
> callback) pair, however need to figure out how to have minimum amount of
> type casting.

David, I thought about this a bit and here is my proposal.

Define "typeful" callback pointer (public):

union rte_ifpx_cb_ptr {
    int (*mac_change)(const struct mac_change *ev);
    int (*mtu_change)(const struct mtu_change *ev);
    ...
    int (*cfg_done)(void);
};

In implementation make sure its size is as expected:

_Static_assert(sizeof(union rte_ifpx_cb_ptr) == sizeof (int(*)(void*)),
               "Size of callback pointer has to be"
               "equal to size of function pointer");

Accept as input tagged callbacks (also public type):

struct rte_ifpx_callback {
    enum rte_ifpx_event_type type;
    union rte_ifpx_cb_ptr callback;
};

The user would be defining array of callbacks:

struct rte_ifpx_callback callbacks[] = {
    {RTE_IFPX_MAC_CHANGE, {.mac_change = mac_change}},
    {RTE_IFPX_MTU_CHANGE, {.mtu_change = mtu_change}},
    ...
    {RTE_IFPX_CFG_DONE,   {.cfg_done   = finished}},
};

and passing it to registration together with its length like:

int rte_ifpx_callbacks_register(int len,
                                const struct rte_ifpx_callback *cbs)
{
    for (int i = 0; i < len; ++i) {
        switch (cbs[i].type) {
            case RTE_IFPX_MAC_CHANGE:
                priv_cbs.mac_change = cbs[i].callback.mac_change;
                break;
    ...
}

This way we should be protected from ABI breakage when adding new event
types and how the callbacks are stored would not be visible to the user.

Let me know what do you think about it.

With regards
Andrzej Ostruszka