Skip to content
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

Fix certificate deletion for array type certificates #1344

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

hongzhidao
Copy link
Contributor

Previously, the certificate deletion only handled string type certificates, causing issues when certificates were specified as an array in the configuration.

@hongzhidao
Copy link
Contributor Author

Hi @andrey-zelenkov,
Welcome to improve tests in your way.

@ac000
Copy link
Member

ac000 commented Jul 2, 2024

Could we split the test change out into its own commit? I know it's currently just a single line change... but I prefer these to be done in separate steps...

@@ -1931,10 +1931,27 @@ nxt_controller_cert_in_use(nxt_str_t *name)
continue;
}

nxt_conf_get_string(value, &str);
if (nxt_conf_type(value) == NXT_CONF_ARRAY) {
n = nxt_conf_array_elements_count(value);
Copy link
Member

Choose a reason for hiding this comment

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

You know I always like to reduce the scope of variables...

We could declare n here...


if (nxt_strstr_eq(&str, name)) {
return 1;
for (i = 0; i < n; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we could now take advantage of C99 for loop initialisers and reduce the scope of i with

for (uint32_t i = 0; i < n; i++) {

if (nxt_strstr_eq(&str, name)) {
return 1;
for (i = 0; i < n; i++) {
element = nxt_conf_get_array_element(value, i);
Copy link
Member

Choose a reason for hiding this comment

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

... also element could be declared here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with both styles but I'll do the same style unit now.

Copy link
Member

Choose a reason for hiding this comment

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

I do think it's a little than just a style choice, see for example.

But I'll leave it up to you...

@hongzhidao
Copy link
Contributor Author

Could we split the test change out into its own commit? I know it's currently just a single line change... but I prefer these to be done in separate steps...

Sure.
I want to ask @alejandro-colomar's help to have a better test patch.

@alejandro-colomar
Copy link
Contributor

Could we split the test change out into its own commit? I know it's currently just a single line change... but I prefer these to be done in separate steps...

Sure. I want to ask @alejandro-colomar's help to have a better test patch.

Hi @hongzhidao !

I guess you meant @andrey-zelenkov ? :-)

@hongzhidao
Copy link
Contributor Author

Could we split the test change out into its own commit? I know it's currently just a single line change... but I prefer these to be done in separate steps...

Sure. I want to ask @alejandro-colomar's help to have a better test patch.

Hi @hongzhidao !

I guess you meant @andrey-zelenkov ? :-)

Oh, you are right. I meant to tag @andrey-zelenkov. Thanks for catching that!

Previously, the certificate deletion only handled string type
certificates, causing issues when certificates were specified
as an array in the configuration.

Reviewed-by: Andrew Clayton <[email protected]>
@hongzhidao hongzhidao merged commit 3621352 into nginx:master Jul 3, 2024
21 of 23 checks passed
@hongzhidao
Copy link
Contributor Author

Hi @ac000,
Merged with your review tag, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants