Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ims_qos, ima_registrar_pcscf: fixes for de-registration and adding a flow to registration #721

Merged
merged 3 commits into from Jul 21, 2016

Conversation

vingarzan
Copy link
Contributor

ims_qos: create a proper flow description for the SIP signaling

  • before there was just an empty one in the AAR
  • re-using the function typically used for the RTP media
  • fixing then that function to not just hard-code protocol 17 (UDP) for everything
  • now supporting also 6 (TCP) and IP (IP has no protocol number, we use just the
    magic word "ip" (see RFC 3588 IPFilterRule for more details)
  • adding the IP of the P-CSCF as a parameter to the module, as this is required
    in the flow (UE IP <-> P-CSCF IP)
  • also some inline/static fixes to function for avoiding warnings on gcc >=5

ims_registrar_pcscf: fixed removal of contacts on de-registration

  • the code was there, but I guess some API changes resulted in temporary
    commented-out code; this uses the new parameters and seems to fix the
    issue.

- before there was just an empty one in the AAR
- re-using the function typically used for the RTP media
- fixing then that function to not just hard-code protocol 17 (UDP) for everything
- now supporting also 6 (TCP) and IP (IP has no protocol number, we use just the
magic word "ip" (see RFC 3588 IPFilterRule for more details)
- adding the IP of the P-CSCF as a parameter to the module, as this is required
in the flow (UE IP <-> P-CSCF IP)
- also some inline/static fixes to function for avoiding warnings on gcc >=5
- the code was there, but I guess some API changes resulted in temporary
commented-out code; this uses the new parameters and seems to fix the
issue.
@ngvoice
Copy link
Member

ngvoice commented Jul 20, 2016

Hi Dragos,

looks okay to me, waiting for @jaybeepee or @richardgood to approve, too.
Can you add the new parameter to the docs, too?

Thanks,
Carsten

@vingarzan
Copy link
Contributor Author

Hi Carsten,

fixed the missing doc. I also changed the name to something hopefully more relevant. "rx_" was for parameters related to the Diameter interface.

Cheers,
-Dragos

@richardgood
Copy link
Contributor

Thanks @vingarzan

All looks good - just want to check one thing with @jaybeepee on ims_registrar_pcscf: fixed removal of contacts on de-registration

I have a sneaking suspicion this was removed on purpose and the contact removal is done through NOTIFY from S-CSCF.

Though I don't think this would cause any issues having it here as well (and could be useful in the event of P-CSCF not implementing subscribe to reg event)

@vingarzan
Copy link
Contributor Author

That's what I thought also, that reginfo would save the day. Then spent a long time tracking down structure of pua, etc, just to find out that the NOTIFY is not doing it unfortunately. Or maybe we missed something...

Anyway: my recommendation is to do both really, to make sure that you catch it even if the configuration has the SUBSCRIBE disabled or some strange S-CSCFs would reject the SUBSCRIBE.

@richardgood
Copy link
Contributor

Hi

Have a look at ims_registrar_pcscf/notify.c line 205.

If the contact is in usrloc and a NOTIFY is received for terminate it should remove it.

But agreed we should do both - just in case. Give me a day or two I just want to double confirm with @jaybeepee why it was removed in the first place to ensure it doesn't have any unintended consequences.

@richardgood
Copy link
Contributor

Ahh I see the confusion - we call reginfo_handle_notify from cfg file - which might not be the case for all P-CSCF cfg files. I think we still need to do quite a bit of polishing on this code and config.

@richardgood richardgood merged commit 476d688 into kamailio:master Jul 21, 2016
@vingarzan
Copy link
Contributor Author

Can i haz the pcscf example file plz :) ?

@richardgood
Copy link
Contributor

I see the reginfo_handle_notify is actually in the example pcscf config file in git:

examples/pcscf/kamailio.cfg line 1184.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants