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
Improves CTLOG_STORE setters #1408
Conversation
Changes them to have clearer ownership semantics, as suggested in openssl#1372 (comment).
+1 |
0666625
to
cf314fa
Compare
{ | ||
ctx->cert = cert; | ||
if (X509_up_ref(cert)) |
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.
If they can fail then they should no longer be void.
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.
Done.
They may fail if they cannot increment the reference count of the certificate they are storing a pointer for. They should return 0 if this occurs.
still +1 needs another reviewer. |
@@ -291,7 +291,7 @@ CRYPTO_gcm128_setiv 291 1_1_0 EXIST::FUNCTION: | |||
ASN1_PCTX_set_oid_flags 292 1_1_0 EXIST::FUNCTION: | |||
d2i_ASN1_INTEGER 293 1_1_0 EXIST::FUNCTION: | |||
i2d_PKCS7_ENCRYPT 294 1_1_0 EXIST::FUNCTION: | |||
CT_POLICY_EVAL_CTX_set0_issuer 295 1_1_0 EXIST::FUNCTION:CT | |||
CT_POLICY_EVAL_CTX_set0_issuer 295 1_1_0 NOEXIST::FUNCTION: |
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.
you could delete these since they now never existed (not sure if we have a script to clean them up before the release)
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.
Should I leave a gap in the number sequence or replace them with the new set1 functions?
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.
Put the new functions directly in place of the old ones. Then I’ll merge.
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.
Done.
+1 |
merged, thanks. closing. |
Changes them to have clearer ownership semantics, as suggested in #1372 (comment). Reviewed-by: Emilia Käsper <emilia@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #1408)
They may fail if they cannot increment the reference count of the certificate they are storing a pointer for. They should return 0 if this occurs. Reviewed-by: Emilia Käsper <emilia@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #1408)
…o.num Reviewed-by: Emilia Käsper <emilia@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #1408)
Changes them to have clearer ownership semantics, as suggested in
#1372 (comment).