Skip to content

x509 parser cleanup - #3582

Closed
bagder wants to merge 3 commits into
masterfrom
bagder/x509-cleanup
Closed

bagder wants to merge 3 commits into
masterfrom
bagder/x509-cleanup

Conversation

@bagder

@bagder bagder commented Feb 18, 2019

Copy link
Copy Markdown
Member

No description provided.

- rename 'n' to buflen in functions, and use size_t for them. Don't pass
  in negative buffer lengths.

- move most function comments to above the function starts like we use
  to

- remove several unnecessary typecasts (especially of NULL)
@bagder
bagder force-pushed the bagder/x509-cleanup branch from 8cf99ff to d9ee323 Compare February 18, 2019 22:05
@bagder
bagder requested a review from monnerat February 18, 2019 22:18
Comment thread lib/x509asn1.c

@monnerat monnerat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except the remark at line 439 that requests a fix, I don't see any other potential problems.
Casted NULLs were coded in old days when the OS400 compiler didn't like implicit type conversion of NULL in some cases. I hope they fixed it nowadays.
Scan-build seems happy with it: now let's see what Coverity says !

@bagder

bagder commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

The travis coverage job fails but I can't understand why, nor do I think it is related to this change.

@monnerat

Copy link
Copy Markdown
Contributor

The travis coverage job fails but I can't understand why, nor do I think it is related to this change.

Agreed, since I have the same problem on #3583. I don't understand the failure reason either.

@bagder bagder closed this in d8b0318 Feb 19, 2019
@bagder
bagder deleted the bagder/x509-cleanup branch February 19, 2019 15:04
@lock lock Bot locked as resolved and limited conversation to collaborators May 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants