Re: Sign changes through function signatures

[Thomas Monjalon <thomas@monjalon.net>]

02/02/2023 21:26, Tyler Retzlaff:
> On Thu, Feb 02, 2023 at 02:23:39PM -0500, Ben Magistro wrote:
> > Hello,
> > 
> > While making some updates to our code base for 22.11.1 that were missed in
> > our first pass through, we hit the numa node change[1].  In the process of
> > updating our code, we noticed that a couple functions (rx/tx_queue_setup,
> > maybe more that we aren't using) state they accept `SOCKET_ID_ANY` but the
> > function signature then asks for an unsigned integer while `SOCKET_ID_ANY`
> > is `-1`.  Following it through the redirect to the "real" function it also
> > asks for an unsigned integer which is then passed on to one or more
> > functions asking for an integer.  As an example using the the i40e driver
> > -- we would call `rte_eth_tx_queue_setup` [2] which ultimately calls
> > `i40e_dev_tx_queue_setup`[3] which finally calls `rte_zmalloc_socket`[4]
> > and `rte_eth_dma_zone_reserve`[5].
> > 
> > I guess what I am looking for is clarification on if this is intentional or
> > if this is additional cleanup that may need to be completed/be desirable so
> > that signs are maintained through the call paths and avoid potentially
> > producing sign-conversion warnings.  From the very quick glance I took at
> > the i40e driver, it seems these are just passed through to other functions
> > and no direct use/manipulation occurs (at least in the mentioned functions).
> 
> i believe this is just sloppyness with sign in our api surface. i too
> find it frustrating that use of these api force either explicit
> casts or suffer having to suppress warnings.
> 
> in the past examples of this have been cleaned up without full deprecation
> notices but there are a lot of instances. i also feel (unpopular opinion)
> that for some integer types like this that have constrained range / number
> spaces it would be of value to introduce a typedef that can be used
> consistently.
> 
> for now you'll just have to add the casts and hopefully in the future we
> will fix the api making them unnecessary. of course feel free to submit
> patches too, it would be great to have these cleaned up.

I agree it should be cleaned up.
Those IDs should accept negative values.
Not sure which type we should choose (int, int32_t, or a typedef).

Another thing to check is the name of the variable.
It should be a socket ID when talking about CPU,
and a NUMA node ID when talking about memory.

And last but not the least,
how can we keep ABI compatibility?
I hope we can use function versioning to avoid deprecation and breaking.

Trials and suggestions are welcome.