Currently core/flags.h defines
typedef unsigned int flag_t;
It would better to use unsigned long in order to allow more that 32 flags on 64 bit architectures.
This has implications on other parts, such as variables where they can be accessed and set via $mf (iirc).
On the other hand, I don't think having a variable with different lengths is a good approach, because it can break easily config behaviour when moving from one server to another. It should be same size on 32b or 64b, so if a change to increase the size is to be done, then should be `long long` to ensure it is 64 on all archs. An alternative would be to make the field inside sip_msg_t an array of flag_t values, so can be one or more, even beyond 64b. But this change will have also impact in other parts, therefore would need careful review.
As an alternative for now would be using an avp or xavps, and do bitwise operations (`|` and `&`) to set or test.
Daniel-Constantin Mierla writes:
This has implications on other parts, such as variables where they can be accessed and set via $mf (iirc).
Other parts of the code should not assume anything about the actual length of flag_t. If they do, that code needs to be fixed.
As an alternative for now would be using an avp or xavps, and do bitwise operations (`|` and `&`) to set or test.
Use of avps for storing bit flags is too slow.
-- Juha
My proposal is to change flag_t to unsigned long long after master is opened for new features. Then there is lots of time to fix code that might get broken.
-- Juha
I wish that changing a structure is something simple, but is not.
The main constraint is having the config interpreter able to deal only with integer numbers as values (static or via variables), and flags field stay behind a variable.
But there could be also memory alignment problems when fields size changes.
Another way could be adding a new field, extended flags, with a different set of functions to handle them like setflagx()/resetflagx(...)/isflagxset(...), so the current behaviour is not affected at all. At this moment, from extensibility point of view I think using an array (static size) is better than single value field. Initially it can be of uint32_t[2] to give access to 64 more flags, but in the future it can be changed if needed. The functions will take two parameters, bit index and array index.
Just some ideas, it can be another way, but I would prefer not to affect that much existing code...
Daniel-Constantin Mierla writes:
Another way could be adding a new field, extended flags, with a different set of functions to handle them like setflagx()/resetflagx(...)/isflagxset(...), so the current behaviour is not affected at all.
That would be fine with me.
view I think using an array (static size) is better than single value field. Initially it can be of uint32_t[2] to give access to 64 more flags, but in the future it can be changed if needed. The functions will take two parameters, bit index and array index.
That would be too complex to use. A flag should be a single integer.
-- Juha
How about using an array of unsigned ints, but functions have only one parameter? Array index is param / 32 and flag number at the index is param % 32.
Just added support for extended flags (xflags) with a capacity of 64 flags. The functions to manage them are exported by corex module. No time to test them, so feedback if all is fine or not is appreciated.
I will also add t_fush_xflags() soon in tmx module.
Daniel-Constantin Mierla writes:
Just added support for extended flags (xflags) with a capacity of 64 flags. The functions to manage them are exported by corex module. No time to test them, so feedback if all is fine or not is appreciated.
I was going to give corex a try, but loading of the module failed like this:
ar 21 01:39:35 trout sip-proxy[13013]: 0(13064) ERROR: <core> [core/sr_module.c:582]: load_module(): could not open module </usr/lib/x86_64-linux-gnu/sip-proxy/modules/corex.so>: /usr/lib/x86_64-linux-gnu/sip-proxy/modules/corex.so: undefined symbol: setxflag
-- Juha
The functions were added by mistake inside a disabled ifdef zone. It should be fixed now in master.
Closed #1288.
Closing, xflags being implemented.
As long as xflags do not work the same way as flags, I do not consider this issue closed. Flags do not require call of t_flush_flags() after t_newtrans() in order to make flags available in failure and onreply routes, but xflags does require call of t_flush_xflags(). This makes xflags cumbersome to use, since I would need to make t_flush_xflags() call before every t_relay() call.
Reopened #1288.
As I wrote on the mailing list, flags should not be migrated to the transaction after t_newtran() unless one uses t_flush_flags() -- that was the desired functionality, otherwise t_flush_flags() has no purpose.
You opened another item on this tracker related to this unexpected behaviour (#1490), and I think it is better to sort this out there, because that is the unexpected behaviour. xflags behave as expected. Whatever will be the conclusion of #1490 will be applied to both flags and xflags, but makes no sense to track an issue with two separate items.
Daniel-Constantin Mierla writes:
As I wrote on the mailing list, flags should not be migrated to the transaction after t_newtran() unless one uses t_flush_flags() -- that was the desired functionality, otherwise t_flush_flags() has no purpose.
I have used flags for many years and for me it has been desirable that they are available after t_newtrans() in failure and onreply routes without a need to call t_flush_flags(). If someone wants to change that behavior and make t_flush_flags() purposeful, then a tm variable can be introduced to achieve that new behavior.
You opened another item on this tracker related to this unexpected behaviour (#1490), and I think it is better to sort this out there, because that is the unexpected behaviour. xflags behave as expected. Whatever will be the conclusion of #1490 will be applied to both flags and xflags, but makes no sense to track an issue with two separate items.
I opened this issue, because I needed more that 32 flags, i.e., flags that work the same way as before, but more of them. It is OK to replace flags with xflags if my current configuration file keeps on working when I textually change current flag function calls with xflag function calls, but it is not OK if I need to start adding t_flush_xflags() calls to it.
Issue "Is t_flush_flags() really needed? #1490" asks about the purpose of t_flush_flags() function. I have not been able to figure out one, but perhaps there is some. If there is no purpose, then the function should be removed.
-- Juha
The purpose of that t_flush_flags() is documented:
* https://www.kamailio.org/docs/modules/stable/modules/tmx.html#tmx.f.t_flush_...
The role was to push the flags set after t_newtran() to transaction. If that is not happening/is not needed for what so ever reason, needs to be sorted out in a way or another.
The behaviour can stay as it is, with or without any new parameter -- this is going to be discussed. But again, it doesn't belong here, the issue we discuss is with flushing the flags after t_newtrans() and should be done in a single place. The xflags will follow whatever is decided for t_flush_flags(), but right now xflags works as it is expected based on docs for t_flush_flags().
Let's discuss further on #1490 and get the two work the same in a properly documented behaviour.
Daniel-Constantin Mierla writes:
The purpose of that t_flush_flags() is documented:
The role was to push the flags set after t_newtran() to transaction. If that is not happening/is not needed for what so ever reason, needs to be sorted out in a way or another.
Yes, and that is the topic of #1490.
The behaviour can stay as it is, with or without any new parameter -- this is going to be discussed. But again, it doesn't belong here, the issue we discuss is with flushing the flags after t_newtrans() and should be done in a single place. The xflags will follow whatever is decided for t_flush_flags(), but right now xflags works as it is expected based on docs for t_flush_flags().
Let's discuss further on #1490 and get the two work the same in a properly documented behaviour.
OK, I'll close this issue with the note that the current semantics of xflags does not correspond to that of flags.
-- Juha
Closing the issue even when xflags as currently implemented do not behave the same way as flags and thus cannot be used as their replacement.
Closed #1288.
-
Daniel-Constantin Mierla -
Juha Heinanen -
juha-h