Re: [PATCH v2] tools: mkeficapsule: Add disable pkcs11 menu option

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Wojciech,

On 4/21/26 10:30 AM, Wojciech Dubowik wrote:
> On Mon, Apr 20, 2026 at 12:16:38PM +0200, Quentin Schulz wrote:
[...]
>> On 4/20/26 10:38 AM, Wojciech Dubowik wrote:
[...]
>>> +#endif
>>>    	int ret;
>>>    	bool pkcs11_cert = false;
>>>    	bool pkcs11_key = false;
>>> @@ -242,6 +244,7 @@ static int create_auth_data(struct auth_context *ctx)
>>>    	if (!strncmp(ctx->key_file, "pkcs11:", strlen("pkcs11:")))
>>>    		pkcs11_key = true;
>>> +#ifndef CONFIG_MKEFICAPSULE_DISABLE_PKCS11
>>>    	if (pkcs11_cert || pkcs11_key) {
>>>    		lib = getenv("PKCS11_MODULE_PATH");
>>>    		if (!lib) {
>>> @@ -259,6 +262,7 @@ static int create_auth_data(struct auth_context *ctx)
>>>    			return -1;
>>>    		}
>>>    	}
>>> +#endif
>>
>> This is getting kinda ugly. I'm wondering if it wouldn't be more readable to
>> move the pkcs11-specific code into specific functions. You call the function
>> from create_auth_data() and you have two definitions of the function, one
>> when CONFIG_MKEFICAPSULE_DISABLE_PKCS11 is enabled, one for when it's not.
>>
> 
> Well. The idea behind was that you can have mixed pkcs11/cert files when creating
> capsule. This is real use case as some HSM are too expensive to store public stuff.
> Rearranging it would go well behind solving the current problem of OE not being able
> to compile. I can have a look into it but probably not before we solve the current
> problem.
> 

Please read the example provided below. The logic is kept intact, it's 
just that the code within if-blocks is moved to a separate function 
instead of having it entirely ifdef'ed within the caller. There's also 
added benefit that if it turns out there are more callers in the future, 
we don't need to duplicate this ifdefery in each caller.

Fixing a bug is not a reason for doing things hastily or not as nice as 
we could do it. I'm not the maintainer though, so this is just me 
sharing some opinion.

>> Something like
>>
>> #if CONFIG_IS_ENABLED(MKEFICAPSULE_DISABLE_PKCS11)
>> static int mkeficapsule_import_pkcs11_crt(...)
>> {
>>      fprintf(stdout, "Pkcs11 support is disabled\n");
>>      return -1;
>> }
>> #else
>> static int mkeficapsule_import_pkcs11_crt(...)
>> {
>> [...]
>> }
>> #endif
>>
>> [...]
>>
>> static int create_auth_data(struct auth_context *ctx)
>> {
>> [...]
>>
>>      if (pkcs11_cert) {
>>          ret = mkeficapsule_import_pkcs11_crt(...);
>>          if (ret < 0) {
>>              fprintf(stdout, "Failed to import crt: %d\n", ret);
>>              return ret;
>>          }
>>      }
>> [...]
>> }

Cheers,
Quentin