Skip to content

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
@anoopcs9 anoopcs9 deleted the rbd-encryption-mem-improvements branch May 9, 2025 18:40
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