-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[ssl] Use grpc_core::Slice in LoadSystemRoots. #36045
base: master
Are you sure you want to change the base?
Conversation
@markdroth PTAL when you have a few moments. :) |
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.
Thanks for doing this cleanup!
|
||
// Returns a slice containing roots from the OS trust store | ||
grpc_slice LoadSystemRootCerts(); | ||
Slice LoadSystemRootCerts(); |
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.
While we're at it, how about changing this to return absl::StatusOr<Slice>
, so that we have a more obvious way of returning an error?
@@ -62,13 +62,13 @@ const char* kCertFiles[] = {"/etc/ssl/cert.pem"}; | |||
const char* kCertDirectories[] = {""}; | |||
#endif // GPR_APPLE | |||
|
|||
grpc_slice GetSystemRootCerts() { | |||
Slice GetSystemRootCerts() { | |||
size_t num_cert_files_ = GPR_ARRAY_SIZE(kCertFiles); |
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.
Pre-existing nit: This should not have an underscore suffix, since it's not a class data member.
@@ -135,26 +135,25 @@ grpc_slice CreateRootCertsBundle(const char* certs_directory) { | |||
} | |||
} | |||
} | |||
bundle_slice = grpc_slice_new(bundle_string, bytes_read, gpr_free); | |||
return bundle_slice; | |||
return Slice(grpc_slice_new(bundle_string, bytes_read, gpr_free)); |
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.
This should be:
return Slice::FromCopiedBuffer(bundle_string, bytes_read);
This is some cleanup that was identified during the review of #34874.