On 24.11.17 01:56, Juha Heinanen wrote:
When adding transport proto check to add_contact_alias, I noticed that in receive_info struct proto is defined like this:
char proto;
and in sip_uri struct like this:
unsigned short proto; /*!< from transport */
That does not look good to me. They should have the same type in both.
I also noticed a mismatch on the type of the same field in different structures, in some cases it seems it was done to have the fields properly aligned in memory, otherwise accessing it via &struct.field can give a SIGBUS on some architectures.
I am fine to get such fields to the same type, just be careful to have them aligned to 32bits from the start of the structure.
Somehow I got to the conclusion that most of the shorter-than-sizeof(int) fields should be converted to int, there are not too many and the overall size increase is not relevant, being safer on avoiding a sigbus when one adds a new field. Quite often is hard to predict the impact of adding a non 32b or 64b field in a structure, because that structure can be used for fields in other structures, so even not accessing that field by its address can mess up the access of other fields from other structures.
In some cases I noticed that a char or short int is also used to store values which are defined in an enum, on the other hand an enum field is stored as integer, so again a mismatch of size on propagating values from fields with same purpose -- these I tried to fix in the past, as some code analyzers could detect them, I am giving it as an example that having at least the int size is a good/safe option.
Back this specific case -- the proto -- I expect there are many other structures having it. It might be good to define a type for it and then change as discovered -- for example:
typedef uint32_t sr_proto_t;
Cheers, Daniel