[2/3] gtphub: fix number map range for TEIs.

Use unsigned int for nr_map, just large enough to fit the TEI space.
Adjust log output formats and casts accordingly.

Fixes: TEIs are uint32_t, but the nr_map so far used int. This would cause TEIs
from 0x80000000 on to be handled and printed as a negative value.

Sponsored-by: On-Waves ehi
---
 openbsc/include/openbsc/gtphub.h   |  2 +-
 openbsc/src/gprs/gtphub.c          | 15 ++++++++-------
 openbsc/tests/gtphub/gtphub_test.c |  6 +++---
 3 files changed, 12 insertions(+), 11 deletions(-)

> On 17 Nov 2015, at 14:38, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> Use unsigned int for nr_map, just large enough to fit the TEI space.
> Adjust log output formats and casts accordingly.
> 
> Fixes: TEIs are uint32_t, but the nr_map so far used int. This would cause TEIs
> from 0x80000000 on to be handled and printed as a negative value.


good!


> -typedef int nr_t;
> +typedef unsigned int nr_t;

use uint32_t here then? and "nr_t" is that clear it refers to a tei?


> +	    (unsigned int)nrm->orig, (unsigned int)nrm->repl);

What is the explicit casting about?

holger

On Wed, Nov 25, 2015 at 08:53:45PM +0100, Holger Freyther wrote:
> > -typedef int nr_t;
> > +typedef unsigned int nr_t;
> 
> use uint32_t here then? and "nr_t" is that clear it refers to a tei?

nr_map is used for both sequence numbers and TEIs with different range
settings. Sequence numbers range from 0..ffff and TEIs from 1..ffffffff.
So nr_t is intended to be more general, but since gtphub is the only scope
using it, unsigned int is enough to fit both number spaces (assuming int
size will never be less than 32 bits...). So nr_t refers to a TEI as soon
as a nr_pool is *used* for a TEI.

> > +	    (unsigned int)nrm->orig, (unsigned int)nrm->repl);
> 
> What is the explicit casting about?

To print values 80000000..ffffffff as positive numbers, the printf format
is using '%u'. Casting to unsigned int it is my preferred way of making
sure the argument matches the printf format...

Might not be necessary, but erases all doubt about intention or type
sizes.

~Neels

On 26.11.2015 17:29, Neels Hofmeyr wrote:
> On Wed, Nov 25, 2015 at 08:53:45PM +0100, Holger Freyther wrote:
> 
>>> +	    (unsigned int)nrm->orig, (unsigned int)nrm->repl);
>>
>> What is the explicit casting about?
> 
> To print values 80000000..ffffffff as positive numbers, the printf format
> is using '%u'. Casting to unsigned int it is my preferred way of making
> sure the argument matches the printf format...
> 
> Might not be necessary, but erases all doubt about intention or type
> sizes.

As far as I understand the C99 specification, this not necessary. IMO
the make the code less readable and the hinder the compiler and tools
like coverity to find certain bugs, e.g. if someone raised the type of
nmr->orig to 'long long' for some reason, he/she wouldn't get a warning
from the tools.

Here is the definition of "integer promotions":

  §6.3.1.1 (2): "If an int can represent all values of the original
type, the value is converted to an int; otherwise, it is converted to an
unsigned int. These are called the integer promotions. All other types
are unchanged by the integer promotions."

Here is the definition of "default argument promotions":

  §6.5.2.2 (6): "If the expression that denotes the called function has
a type that does not include a prototype, the integer promotions are
performed on each argument, and arguments that have type float are
promoted to double. These are called the default argument promotions. ..."

And finally, these have to be applied to the trailing arguments or printf:

  §6.5.2.2 (7): "... The ellipsis notation in a function prototype
declarator causes argument type conversion to stop after the last
declared parameter. The default argument promotions are performed on
trailing arguments."

Jacob