Currently add_contact_alias() works like this:
Adds an “;alias=ip~port~transport” parameter to the contact URI containing either received ip, port, and transport protocol or those given as parameters. If called without parameters, “;alias” parameter is only added if received ip and port differ from those in contact URI.
Now I noticed that a broken UA does not add a proper ;transport param to contact uri and therefore received transport does not match contact uri transport. Therefore transport proto does not match, but ip and port do match.
It would be useful is add_contact_alias() would add ;alias param also if transports do not match. Is it OK if I make the change?
After that README would read:
Adds an “;alias=ip~port~transport” parameter to the contact URI containing either received ip, port, and transport protocol or those given as parameters. If called without parameters, “;alias” parameter is only added if received ip, port, and transport protocol differ from those in contact URI.
-- Juha
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.
-- Juha
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
Daniel-Constantin Mierla writes:
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.
I'm not qualified enough to do that kind change, since it may affect many parts of the core and many modules.
-- Juha
On 24.11.17 01:26, Juha Heinanen wrote:
Currently add_contact_alias() works like this:
Adds an “;alias=ip~port~transport” parameter to the contact URI containing either received ip, port, and transport protocol or those given as parameters. If called without parameters, “;alias” parameter is only added if received ip and port differ from those in contact URI.
Now I noticed that a broken UA does not add a proper ;transport param to contact uri and therefore received transport does not match contact uri transport. Therefore transport proto does not match, but ip and port do match.
It would be useful is add_contact_alias() would add ;alias param also if transports do not match. Is it OK if I make the change?
After that README would read:
Adds an “;alias=ip~port~transport” parameter to the contact URI containing either received ip, port, and transport protocol or those given as parameters. If called without parameters, “;alias” parameter is only added if received ip, port, and transport protocol differ from those in contact URI.
Transport should be taken in consideration, indeed. You can go ahead and do it, then can be backported to stable branches.
Cheers, Daniel
-
Daniel-Constantin Mierla -
Juha Heinanen