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

[Jakub Jelen]

On Mon, 2018-08-06 at 13:08 +0200, Marc-André Lureau wrote:
> 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?

Ack. This looks good. Thanks.

-- 
Jakub Jelen
Software Engineer
Security Technologies
Red Hat, Inc.