[Asterisk-code-review] STIR/SHAKEN: Option split and response codes. (asterisk[16])

[George Joseph]

Attention is currently required from: Joshua Colp, Kevin Harwell, Benjamin Keith Ford.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16497 )

Change subject: STIR/SHAKEN: Option split and response codes.
......................................................................


Patch Set 6: Code-Review-1

(3 comments)

File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/585f80c2_284e1ccf 
PS6, Line 516: enum ast_sip_stir_shaken_behavior {
             : 	/*! Don't do any STIR/SHAKEN operations */
             : 	AST_SIP_STIR_SHAKEN_OFF = (1 << 0),
             : 	/*! Only do STIR/SHAKEN attestation */
             : 	AST_SIP_STIR_SHAKEN_ATTEST = (1 << 1),
             : 	/*! Only do STIR/SHAKEN verification */
             : 	AST_SIP_STIR_SHAKEN_VERIFY = (1 << 2),
             : 	/*! Do STIR/SHAKEN attestation and verification */
             : 	AST_SIP_STIR_SHAKEN_ON = (1 << 3),
             : };
             : 
             : /
I don't think this is right.  I think it should be 
enum ast_sip_stir_shaken_behavior {
	/*! Don't do any STIR/SHAKEN operations */
	AST_SIP_STIR_SHAKEN_OFF = 0,
	/*! Only do STIR/SHAKEN attestation */
	AST_SIP_STIR_SHAKEN_ATTEST = 1,
	/*! Only do STIR/SHAKEN verification */
	AST_SIP_STIR_SHAKEN_VERIFY = 2,
	/*! Do STIR/SHAKEN attestation and verification */
	AST_SIP_STIR_SHAKEN_ON = 3,
};

This way you can do
    if (testvalue & AST_SIP_STIR_SHAKEN_ATTEST)
or
    if (testvalue & AST_SIP_STIR_SHAKEN_VERIFY)

If you leave them the way they are then AST_SIP_STIR_SHAKEN_ON isn't a bitmap of ATTEST and VERIFY.


File res/res_pjsip_session.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/d08ced76_54c759f2 
PS6, Line 4071: 	if ((endpoint->stir_shaken == AST_SIP_STIR_SHAKEN_VERIFY ||
              : 		endpoint->stir_shaken == AST_SIP_STIR_SHAKEN_ON) &&
This is what I mean...  You can reduce this to 
if (endpoint->stir_shaken & AST_SIP_STIR_SHAKEN_VERIFY )


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/16497/comment/f60a03a8_f376eed1 
PS6, Line 476: (session->endpoint->stir_shaken & (AST_SIP_STIR_SHAKEN_ATTEST | AST_SIP_STIR_SHAKEN_ON))
Again, can be reduced to
if (session->endpoint->stir_shaken & AST_SIP_STIR_SHAKEN_ATTEST)

You could even create a few macros...

#define AST_STIR_SHAKEN_MUST_VERIFY(option) (option & AST_SIP_STIR_SHAKEN_VERIFY)
#define AST_STIR_SHAKEN_MUST_ATTEST(option) (option & AST_SIP_STIR_SHAKEN_ATTEST)

if (AST_STIR_SHAKEN_MUST_ATTEST(session->endpoint->stir_shaken))



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16497
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I4ac1ecf652cd0e336006b0ca638dc826b5b1ebf7
Gerrit-Change-Number: 16497
Gerrit-PatchSet: 6
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Benjamin Keith Ford <bford at digium.com>
Gerrit-Comment-Date: Thu, 21 Oct 2021 14:51:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211021/b212b08c/attachment.html>