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

crypto: Add support for LUKS Reencryption. #1065

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xyakimo1
Copy link
Contributor

No description provided.

src/plugins/crypto.c Outdated Show resolved Hide resolved
@xyakimo1 xyakimo1 marked this pull request as draft September 28, 2024 12:26
src/plugins/crypto.c Outdated Show resolved Hide resolved
@xyakimo1 xyakimo1 marked this pull request as ready for review September 30, 2024 23:02
* Returns: 0, if the reencryption should continue.
* A non-zero value to stop the reencryption
*/
typedef int (*BDCryptoLUKSReencryptProgFunc) (guint64 size, guint64 offset);
Copy link
Member

Choose a reason for hiding this comment

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

Please provide another argument - gpointer user_data. This is a good practice with Glib callbacks to pass through user data that the caller will use for a back-reference. Nothing special, just pass the value. All the functions that take BDCryptoLUKSReencryptProgFunc in will need to be extended with this gpointer user_data argument as well.

I wasn't able to find any nice writeup, this is perhaps close to what I'm talking about: https://www.geany.org/manual/gtk/gtk-tutorial/x159.html

Better to talk to @vojtechtrefny first, I don't remember exactly what is the style of callbacks in libblockdev.

g_free (params->resilience);
g_free (params->hash);
bd_crypto_luks_pbkdf_free(params->pbkdf);
}
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to free params itself ;-)

BD_CRYPTO_LUKS_REENCRYPT_CLEAN,
BD_CRYPTO_LUKS_REENCRYPT_CRASH,
BD_CRYPTO_LUKS_REENCRYPT_INVALID
} BDCryptoLUKSReencryptStatus;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at bd_crypto_luks_reencrypt_status(), does it make sense to include status values like "running" or "finished"?

Comment on lines +1243 to +1246
BD_CRYPTO_LUKS_REENCRYPT_NONE = 0,
BD_CRYPTO_LUKS_REENCRYPT_CLEAN,
BD_CRYPTO_LUKS_REENCRYPT_CRASH,
BD_CRYPTO_LUKS_REENCRYPT_INVALID
Copy link
Member

Choose a reason for hiding this comment

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

Please also document enum values.

Comment on lines +1266 to +1267
* Returns: true, if the reencryption was successful or gracefully stopped with @prog_func.
* false, if an error occurred.
Copy link
Member

Choose a reason for hiding this comment

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

As this is a long-running operation, I (as an user) would like to know whether this is a blocking call or not. I.e. whether this is a synchronous operation or just a trigger to start an operation in the background. If this is a blocking call, would be good to warn user that this may take hours or days to complete.

Comment on lines +1295 to +1296
* Returns: true, if the reencryption finished successfully or was gracefully stopped with @prog_func.
* false, if an error occurred.
Copy link
Member

Choose a reason for hiding this comment

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

Just like above, would be good to know if this call blocks until the operation is finished or if it just resumes an operation in the background.

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.

2 participants