Conversation
FdaSilvaYY
force-pushed
the
srtp_cleanup
branch
from
af7d446 to
bf7ba2c
Compare
apps/s_server.c
Outdated
apps/s_client.c
Outdated
There was a problem hiding this comment.
Why not put this inside an #ifdef too?
There was a problem hiding this comment.
Because it now becomes just a local variable instead of a global one.
The two places you note must be guarded together (or not), else compilers will raise an unused variable warnings.
Disabling the command-line option declaration and the use place of srtp_profiles is enough ... we can afford an unused pointer.
There was a problem hiding this comment.
I would prefer to get an error that it's not supported instead of parsing something and then just ignoring it.
There was a problem hiding this comment.
@kroeckx : I just try to simplify the handling of NO_SRTP and make it more similar to the other options like NO_SRP or NO_PSK ...
None of the disable options are actually triggering an error message.
FdaSilvaYY
force-pushed
the
srtp_cleanup
branch
from
bf7ba2c to
f47d194
Compare
Remove two unneeded global variables: my 2'cents to openssl#4679
FdaSilvaYY
force-pushed
the
srtp_cleanup
branch
from
f47d194 to
26b7aaf
Compare
richsalz
added
the
approval: review pending
kaduk
added
approval: done
|
I would like another reviewer (I dislike two from the same employer), especially since @kroeckx had issues with this. |
|
On Wed, Mar 21, 2018 at 05:27:59AM -0700, Rich Salz wrote:
I would like another reviewer (I dislike two from the same employer), especially since @kroeckx had issues with this.
Feel free to merge this, but I would rather see a consistent style
that options that are not supported return an error message.
|
|
I agree, unsupported options should return an error message. The right way to do this is leave the code in the switch, and leave the enum definitions. Just remove the options from the big table. That way the 'opt-next' code can never return one of those options, it will instead return an error. |
|
.. which is to say that the code does the right thing. :) |
|
... the code will do the right thing after this PR ;) |
|
So @kroeckx are you okay with being listed as a reviewer? If not, I'd like someone additional before I merge. (Of course, someone else can merge if they don't feel as strongly.) |
|
Don't put me as reviewer.
|
|
Okay, fair enough. Someone else can merge this, or someone else needs to review before I'll merge. |
|
Merged to master, thanks! |
Add missing guards around STRP-related fields Remove two unneeded global variables: my 2'cents to #4679 Merge definition and instantiation of srpsrvparm global. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Ben Kaduk <[email protected]> Reviewed-by: Rich Salz <[email protected]> (Merged from #4908)
5 participants
The code-guards around code and variables for this extension can be improved.
Remove two unneeded global variables: my 2'cents to #4679
The "use_srtp" command-line option appears in the command options list, even when his support is disabled ...
Checklist