IS_ENABLED(CONFIG_ENCRYPTED_KEYS) is true whether the option is built-in
or a module, so use it instead of #if defined checking for each
separately.
The other #if was to avoid a static function defined, but unused
warning. As we now always build the callsite when the function
is defined, we can remove that first #if guard.
Suggested-by: Arnd Bergmann <[email protected]>
Signed-off-by: Ahmad Fatoum <[email protected]>
---
Cc: Dmitry Baryshkov <[email protected]>
---
drivers/md/dm-crypt.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 8c874710f0bc..7eeb9248eda5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2436,7 +2436,6 @@ static int set_key_user(struct crypt_config *cc, struct key *key)
return 0;
}
-#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
static int set_key_encrypted(struct crypt_config *cc, struct key *key)
{
const struct encrypted_key_payload *ekp;
@@ -2452,7 +2451,6 @@ static int set_key_encrypted(struct crypt_config *cc, struct key *key)
return 0;
}
-#endif /* CONFIG_ENCRYPTED_KEYS */
static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
{
@@ -2482,11 +2480,10 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
} else if (!strncmp(key_string, "user:", key_desc - key_string + 1)) {
type = &key_type_user;
set_key = set_key_user;
-#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
- } else if (!strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
+ } else if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS) &&
+ !strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
type = &key_type_encrypted;
set_key = set_key_encrypted;
-#endif
} else {
return -EINVAL;
}
--
2.30.0
Hello Mike,
On 02.02.21 19:10, Mike Snitzer wrote:
> On Fri, Jan 22 2021 at 3:43am -0500,
> Ahmad Fatoum <[email protected]> wrote:
>
>> IS_ENABLED(CONFIG_ENCRYPTED_KEYS) is true whether the option is built-in
>> or a module, so use it instead of #if defined checking for each
>> separately.
>>
>> The other #if was to avoid a static function defined, but unused
>> warning. As we now always build the callsite when the function
>> is defined, we can remove that first #if guard.
>>
>> Suggested-by: Arnd Bergmann <[email protected]>
>> Signed-off-by: Ahmad Fatoum <[email protected]>
>> ---
>> Cc: Dmitry Baryshkov <[email protected]>
>> ---
>> drivers/md/dm-crypt.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
>> index 8c874710f0bc..7eeb9248eda5 100644
>> --- a/drivers/md/dm-crypt.c
>> +++ b/drivers/md/dm-crypt.c
>> @@ -2436,7 +2436,6 @@ static int set_key_user(struct crypt_config *cc, struct key *key)
>> return 0;
>> }
>>
>> -#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
>> static int set_key_encrypted(struct crypt_config *cc, struct key *key)
>> {
>> const struct encrypted_key_payload *ekp;
>> @@ -2452,7 +2451,6 @@ static int set_key_encrypted(struct crypt_config *cc, struct key *key)
>>
>> return 0;
>> }
>> -#endif /* CONFIG_ENCRYPTED_KEYS */
>>
>> static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
>> {
>> @@ -2482,11 +2480,10 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
>> } else if (!strncmp(key_string, "user:", key_desc - key_string + 1)) {
>> type = &key_type_user;
>> set_key = set_key_user;
>> -#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
>> - } else if (!strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
>> + } else if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS) &&
>> + !strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
>> type = &key_type_encrypted;
>> set_key = set_key_encrypted;
>> -#endif
>> } else {
>> return -EINVAL;
>> }
>> --
>> 2.30.0
>>
>
> I could be mistaken but the point of the previous way used to enable
> this code was to not compile the code at all. How you have it, the
> branch just isn't taken but the compiled code is left to bloat dm-crypt.
>
> Why not leave this as is and follow same pattern in your next patch?
It's safe to assume the compiler will constant-fold the resulting if (0) away.
The kernel coding style documentation got a section on that:
https://www.kernel.org/doc/html/v5.11-rc6/process/coding-style.html#conditional-compilation
Cheers,
Ahmad
>
> Mike
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Fri, Jan 22 2021 at 3:43am -0500,
Ahmad Fatoum <[email protected]> wrote:
> IS_ENABLED(CONFIG_ENCRYPTED_KEYS) is true whether the option is built-in
> or a module, so use it instead of #if defined checking for each
> separately.
>
> The other #if was to avoid a static function defined, but unused
> warning. As we now always build the callsite when the function
> is defined, we can remove that first #if guard.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Ahmad Fatoum <[email protected]>
> ---
> Cc: Dmitry Baryshkov <[email protected]>
> ---
> drivers/md/dm-crypt.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 8c874710f0bc..7eeb9248eda5 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2436,7 +2436,6 @@ static int set_key_user(struct crypt_config *cc, struct key *key)
> return 0;
> }
>
> -#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
> static int set_key_encrypted(struct crypt_config *cc, struct key *key)
> {
> const struct encrypted_key_payload *ekp;
> @@ -2452,7 +2451,6 @@ static int set_key_encrypted(struct crypt_config *cc, struct key *key)
>
> return 0;
> }
> -#endif /* CONFIG_ENCRYPTED_KEYS */
>
> static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
> {
> @@ -2482,11 +2480,10 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
> } else if (!strncmp(key_string, "user:", key_desc - key_string + 1)) {
> type = &key_type_user;
> set_key = set_key_user;
> -#if defined(CONFIG_ENCRYPTED_KEYS) || defined(CONFIG_ENCRYPTED_KEYS_MODULE)
> - } else if (!strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
> + } else if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS) &&
> + !strncmp(key_string, "encrypted:", key_desc - key_string + 1)) {
> type = &key_type_encrypted;
> set_key = set_key_encrypted;
> -#endif
> } else {
> return -EINVAL;
> }
> --
> 2.30.0
>
I could be mistaken but the point of the previous way used to enable
this code was to not compile the code at all. How you have it, the
branch just isn't taken but the compiled code is left to bloat dm-crypt.
Why not leave this as is and follow same pattern in your next patch?
Mike
пт, 22 янв. 2021 г. в 11:43, Ahmad Fatoum <[email protected]>:
>
> IS_ENABLED(CONFIG_ENCRYPTED_KEYS) is true whether the option is built-in
> or a module, so use it instead of #if defined checking for each
> separately.
>
> The other #if was to avoid a static function defined, but unused
> warning. As we now always build the callsite when the function
> is defined, we can remove that first #if guard.
>
> Suggested-by: Arnd Bergmann <[email protected]>
> Signed-off-by: Ahmad Fatoum <[email protected]>
Acked-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry