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_registrar_scscf: Remove buggy AVP from SAR #659

Closed
wants to merge 1 commit into from

Conversation

fedefava86
Copy link

Remove buggy function to add
"Call-ID" AVP which actually doesn't exists
on SAR Diameter Message
According to TS129.229, it actually exists
"Call-ID-Sip-Header" AVP witch code 643
which is grouped into "Subscription-Info"
AVP. According to the same TS, this AVP
is used for restoration procedure.

Remove buggy function to add
"Call-ID" AVP which actually doesn't exists
on SAR Diameter Message
According to TS129.229, it actually exists
"Call-ID-Sip-Header" AVP witch code 643
which is grouped into "Subscription-Info"
AVP. According to the same TS, this AVP
is used for restoration procedure.
@miconda
Copy link
Member

miconda commented Jun 7, 2016

I guess @jaybeepee, @richardgood or @ngvoice want to check before merging.

@kamailio-sync
Copy link

yup, checking

On Tue, 7 Jun 2016 at 09:20 Daniel-Constantin Mierla <
notifications@github.com> wrote:

I guess @jaybeepee https://github.com/jaybeepee, @richardgood
https://github.com/richardgood or @ngvoice https://github.com/ngvoice
want to check before merging.

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#659 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AF36ZVGkKEL7OPDFOeSxp2dlbXK99nRyks5qJRuogaJpZM4Iu6pk
.


sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev

@fedefava86
Copy link
Author

Any feedback regarding this ?

@miconda
Copy link
Member

miconda commented Jun 20, 2016

Any comment by @jaybeepee, @richardgood or @ngvoice? Eventually can be made optional with a modparam instead of complete remove if someone still finds it useful for some cases.

@fedefava86
Copy link
Author

Eventually, if really needed, i can try to find some time to modify the Pull Request in order to introduce a modparam. The point is that, as far as i understand, the AVP does not follow any (known to me at least) 3gpp or RFC specs...so is pointless to add it as it is, if not used against a "custom" HSS that needs this AVP

@jaybeepee
Copy link
Contributor

Apologies Frederico, I've been swamped. Will take a look today

@fedefava86
Copy link
Author

Still nothing?

@miconda
Copy link
Member

miconda commented Jun 24, 2016

If you introduce a nod parameters, then I can step in and merge. If someone needs old behaviour, nothing is lost.

@fedefava86
Copy link
Author

Sorry if i insist on the point...but really i do not see the point in leaving such AVP. To me is sufficient a feedback like "Our HSS is caring about this AVP, because we have a custom HSS that....etc...etc". I will close the PR or add a modparam and that's ok. This way we can for sure have a fallback, but basically we are mantaining an "out-of-standard" thing (that, btw, even wireshark doesn't decode) without knowing if it is really needed or not.

Anyway, copy that, with low priority i will try to find some time to add a modparam if really needed

@jaybeepee
Copy link
Contributor

Frederico, the problem is that the code is already in ..... there could be
other ppl using it, so I think it would be unfair to just remove it. We
have 2 solutions here:

  1. move to a modparam.
  2. move it to a vendor-specific AVP

Finally, we will need to find someone who has time/priority to get this
done... if you are worried only about the std, then I think it's a low
priority. If it's affecting your ability to use Kamailio, then it's of
course higher priority.

On Fri, 24 Jun 2016 at 14:34 Federico Favaro notifications@github.com
wrote:

Sorry if i insist on the point...but really i do not see the point in
leaving such AVP. To me is sufficient a feedback like "Our HSS is caring
about this AVP, because we have a custom HSS that....etc...etc". I will
close the PR or add a modparam and that's ok. This way we can for sure have
a fallback, but basically we are mantaining an "out-of-standard" thing
(that, btw, even wireshark doesn't decode) without knowing if it is really
needed or not.

Anyway, copy that, with low priority i will try to find some time to add a
modparam if really needed


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#659 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABI0d_gufKE3DAJL36sFWo9tepQP1C0Qks5qO85EgaJpZM4Iu6pk
.

@jaybeepee
Copy link
Contributor

I've added a mod-param version for now and checked-in to master. When I find some time to dig through specs, I will add the std version.

@fedefava86 fedefava86 closed this Jun 24, 2016
@jaybeepee
Copy link
Contributor

BTW, Frederico, you will see the AVP is marked as vendor-specific already... which is why wireshark will not decode...

Trace some big vendor's diameter interfaces and you will see plenty of non-parseable avps in wireshark traces ;)

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

4 participants