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

rbd: Memory allocation improvements for EncryptionLoad APIs #1078

Merged
merged 2 commits into from
Feb 19, 2025

Conversation

anoopcs9
Copy link
Collaborator

See below for the motivation:

          This could be an Go allocated array, but doesn't hurt either.

Originally posted by @ansiwen in #1061 (comment)

          So, here we still allocate Go memory, but it will be referenced in the `cEncryptionData`. ~I guess, wherever that is used, we still have the same problem.~ Ok, I checked, indeed it is used in a "safe" way, that is `cEncryptionData` is not passed directly as a C argument. But still:  I think it would be safer to `malloc` that as well and add it to the free function, because - as it happened to you this time - , it might happen again that someone falsely assumes, it is completely C allocated.

          However: We can add that in a follow-up (I can do it) if you want to merge it as is for the release.

Originally posted by @ansiwen in #1061 (comment)

For the sake of completeness we add a missing memory allocation for
cOpts inside allocateEncryptionOptions() with the following types:

- C.rbd_encryption_luks1_format_options_t
- C.rbd_encryption_luks2_format_options_t
- C.rbd_encryption_luks_format_options_t

There is also a re-ordering of statements to make it look more similar
to recently added writeEncryptionSpec() for EncryptionLoad2 API.

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
Instead of creating a slice based on a pre-allocated C memory array we
could directly go for a slice created via the built-in function make().

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
@anoopcs9 anoopcs9 changed the title rbd/encryption: Internal memory allocation improvements rbd: Memory allocation improvements for EncryptionLoad APIs Feb 17, 2025
@anoopcs9 anoopcs9 force-pushed the rbd-encryption-mem-improvements branch from b3db13f to aa1e5f4 Compare February 17, 2025 06:27
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

nothing jumps out at me as an issue. Not approving because these come from @ansiwen suggestions and want him to review it.

@phlogistonjohn phlogistonjohn added the no-API This PR does not include any changes to the public API of a go-ceph package label Feb 17, 2025
@phlogistonjohn
Copy link
Collaborator

ping @ansiwen

Copy link
Collaborator

@ansiwen ansiwen left a comment

Choose a reason for hiding this comment

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

Oh, thanks for picking this up! LGTM.

@mergify mergify bot merged commit e84ad6c into ceph:master Feb 19, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-API This PR does not include any changes to the public API of a go-ceph package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants