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

c/topic_config: Explicitly initialize non-class fields #23149

Merged

Conversation

oleiman
Copy link
Member

@oleiman oleiman commented Aug 30, 2024

Since we need a default constructor to keep serde happy.

This PR adds some default initializers to cluster::topic_configuration to avoid UB when the default constructor is used as in this function. We could/should adjust the test code to avoid this pattern, but precluding the invalid state at the struct definition is probably sufficient here.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.2.x
  • v24.1.x
  • v23.3.x

Release Notes

  • none

Since we need a default constructor to keep serde happy.
Copy link
Contributor

@bashtanov bashtanov left a comment

Choose a reason for hiding this comment

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

do we not backport because it's "just tests"?

@mmaslankaprv
Copy link
Member

i think we should still backport it

@oleiman
Copy link
Member Author

oleiman commented Aug 30, 2024

i think we should still backport it

Happy to do so. I didn't realize this code was on older branches.

@oleiman oleiman merged commit 0cfc85f into redpanda-data:dev Aug 30, 2024
20 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

Comment on lines -69 to +73
int32_t partition_count;
int32_t partition_count{0};
// using signed integer because Kafka protocol defines it as signed int
int16_t replication_factor;
int16_t replication_factor{0};
// bypass migration restrictions
bool is_migrated;
bool is_migrated{false};
Copy link
Member

Choose a reason for hiding this comment

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

nice find

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

Successfully merging this pull request may close these issues.

5 participants