[Spice-devel] [PATCH libcacard] options: add use_hw=removable

[Marc-André Lureau]

Hi

On Mon, Aug 6, 2018 at 12:53 PM, Jakub Jelen <jjelen at redhat.com> wrote:
> On Fri, 2018-08-03 at 12:40 +0200, marcandre.lureau at redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>
>> use_hw=yes is used to specify that libcacard will lookup pkcs11 slots
>> that are both removable & hardware.
>>
>> Let's specify that use_hw=removable will select removable slots
>> (without hardware requirement).
>>
>> (use_hw= should eventually be changed/aliased to pk11_slots= or
>> something else)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
>> ---
>>  src/vcard_emul_nss.c | 25 ++++++++++++++++++-------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/vcard_emul_nss.c b/src/vcard_emul_nss.c
>> index 8a56343..99da091 100644
>> --- a/src/vcard_emul_nss.c
>> +++ b/src/vcard_emul_nss.c
>> @@ -77,13 +77,19 @@ struct VirtualReaderOptionsStruct {
>>      int cert_count;
>>  };
>>
>> +enum {
>> +    USE_HW_NO,
>> +    USE_HW_YES,
>> +    USE_HW_REMOVABLE,
>> +};
>> +
>>  struct VCardEmulOptionsStruct {
>>      void *nss_db;
>>      VirtualReaderOptions *vreader;
>>      int vreader_count;
>>      VCardEmulType hw_card_type;
>>      const char *hw_type_params;
>> -    PRBool use_hw;
>> +    int use_hw;
>>  };
>>
>>  static int nss_emul_init;
>> @@ -811,7 +817,7 @@ static const VCardEmulOptions default_options = {
>>      .vreader_count = 0,
>>      .hw_card_type = VCARD_EMUL_CAC,
>>      .hw_type_params = "",
>> -    .use_hw = PR_TRUE
>> +    .use_hw = USE_HW_YES,
>>  };
>>
>>
>> @@ -1037,7 +1043,8 @@ vcard_emul_init(const VCardEmulOptions
>> *options)
>>              PK11SlotInfo *slot = module->slots[i];
>>
>>              /* only map removable HW slots */
>> -            if (slot == NULL || !PK11_IsRemovable(slot) ||
>> !PK11_IsHW(slot)) {
>> +            if (slot == NULL || !PK11_IsRemovable(slot) ||
>> +                (options->use_hw == USE_HW_YES && !PK11_IsHW(slot)))
>> {
>>                  continue;
>>              }
>>              if (strcmp("E-Gate 0 0", PK11_GetSlotName(slot)) == 0) {
>> @@ -1235,9 +1242,12 @@ vcard_emul_options(const char *args)
>>          } else if (strncmp(args, "use_hw=", 7) == 0) {
>>              args = strip(args+7);
>>              if (*args == '0' || *args == 'N' || *args == 'n' ||
>> *args == 'F') {
>> -                opts->use_hw = PR_FALSE;
>> +                opts->use_hw = USE_HW_NO;
>> +            } else if (strncmp(args, "removable", 9) == 0) {
>> +                opts->use_hw = USE_HW_REMOVABLE;
>> +                args = strip(args+9);
>
> Is this strip really needed? After the else, we are skipping any non-
> blank characters so this is practically noop.

You are right it is not strictly required, I can remove it. The
vcard_emul_options() is pretty awful, there is a comment above saying
that we should use some existing argument parsing library...

diff --git a/src/vcard_emul_nss.c b/src/vcard_emul_nss.c
index 99da091..7dd0918 100644
--- a/src/vcard_emul_nss.c
+++ b/src/vcard_emul_nss.c
@@ -1288,12 +1288,12 @@ vcard_emul_options(const char *args)
             }
         } else if (strncmp(args, "nssemul", 7) == 0) {
             opts->hw_card_type = VCARD_EMUL_CAC;
-            opts->use_hw = PR_TRUE;
+            opts->use_hw = USE_HW_YES;
             args = find_blank(args + 7);
 #if defined(ENABLE_PCSC)
         } else if (strncmp(args, "passthru", 8) == 0) {
             opts->hw_card_type = VCARD_EMUL_PASSTHRU;
-            opts->use_hw = PR_TRUE;
+            opts->use_hw = USE_HW_YES;
             args = find_blank(args + 8);
 #endif
         } else {


>
>>              } else {
>> -                opts->use_hw = PR_TRUE;
>> +                opts->use_hw = USE_HW_YES;
>>              }
>>              args = find_blank(args);
>>          /* hw_type= */
>
> Otherwise it looks fine.

ack with the above changes?



-- 
Marc-André Lureau