Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor pem #605
refactor pem #605
Changes from 7 commits
65131c0
3d5efe4
38c68f2
6c7f86e
a5465b6
63d58a3
21f4060
fb30c96
0a8af82
3a53cd0
86187b2
f287efe
844611e
095679c
6e626ab
a74eaab
8484bc2
85b96d1
899cc46
c1e677a
265cc7f
45f7ef0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect other systems to use a different error if the objects themselves are all valid, but the "document" itself is malformed due to missing required objects?
If we fear people will re-use this error-code for bad documents, give it a more generic name like
AWS_ERROR_PEM_MALFORMED
. But if you want distinct error-codes to be created for malformed documents of a specific type, then this is goodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me just make it generic AWS_ERROR_PEM_MALFORMED. i dont think its worth distinguishing that its individual objects that are malformed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debatable: would it be simpler if these functions were alternate array-list initializers? I can't imagine this function ever being used without the user needing to initialize the list first. Like:
int aws_array_list_init_from_pem_file_contents(struct aws_array_list *list, struct aws_allocator, struct aws_byte_cursor pem_contents);
int aws_array_list_init_from_pem_file_path(struct aws_array_list *list, struct aws_allocator, const char *pem_path);
Or call it "pem_objects" instead of "array_list" (I like this idea best)?
int aws_pem_objects_init_from_file_contents(struct aws_array_list *pem_objects, struct aws_allocator, struct aws_byte_cursor pem_contents);
int aws_pem_objects_init_from_file_path(struct aws_array_list *pem_objects, struct aws_allocator, const char *pem_path);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current names were mostly based on existing names for internal function with a modification to refer to elements as pem objects instead of cert_or_key.
I like your suggestion of just naming it pem_objects_init. next commit switches to that.
I think original code wanted array to be initialized outside of the call because the caller might have better context on how many elements are there in the pem file. But in practice the answer to that is either probably just one or probably many, so initializing outside was not super helpful. Moved array init inside of the function.